Adjust type of ChemicalFormulaFeaturizer.powers. #964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The backend is opening up the type of powers to be floats. As such, this commit will deserialize powers as a list of floats. However, to preserve backwards compatability, the type of powers will be preserved, and a new field (powers_float) with the list as floats is introduced. In v4.0.0, the type of 'powers' will be changed, and 'powers_float' will be deprecated before its removal in v5.0.0.
anoto-moniz
force-pushed
the
bugfix/powers-type
branch
from
94d1622 to
a812f20
Compare
kroenlein
previously requested changes
Sep 18, 2024
There was a problem hiding this comment.
I think the migration strategy proposed is overkill given the actual impacts in Python of changing an int to a float. But I will disagree and commit.
Returning incorrect values when the object contains non-integer powers feels like mishandling data. That definitely violates minimum-surprise for me.
src/citrine/informatics/predictors/chemical_formula_featurizer.py
Outdated
Show resolved
Hide resolved
src/citrine/informatics/predictors/chemical_formula_featurizer.py
Outdated
Show resolved
Hide resolved
src/citrine/informatics/predictors/chemical_formula_featurizer.py
Outdated
Show resolved
Hide resolved
anoto-moniz
dismissed
kroenlein’s stale review
The approach suggested has been rejected in conversations via Slack.
kroenlein
reviewed
Sep 18, 2024
src/citrine/informatics/predictors/chemical_formula_featurizer.py
Outdated
Show resolved
Hide resolved
StevenKauwe
previously approved these changes
Sep 18, 2024
There was a problem hiding this comment.
Catching up from OOO. These changes look good to me! 👍
They seem to preserve current client behavior while also adding the ability to work with floats if the user opts into it. Beyond that, looks like a lot already got discussed. This was helpful for catching up!
kroenlein
approved these changes
Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The backend is opening up the type of powers to be floats. As such, this commit will deserialize powers as a list of floats. However, to preserve backwards compatability, the type of powers will be preserved, and a new field (powers_float) with the list as floats is introduced. In v4.0.0, the type of 'powers' will be changed, and 'powers_float' will be deprecated before its removal in v5.0.0.
PR Type:
Adherence to team decisions