[PNE-6367] Add support for feature effects. #982
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
anoto-moniz
commented
Nov 22, 2024
| @@ -88,6 +88,7 @@ class TableConfig(Resource["TableConfig"]): | |||
| The query used to define the materials underpinning this table | |||
| generation_algorithm: TableFromGemdQueryAlgorithm | |||
| Which algorithm was used to generate the config based on the GemdQuery results | |||
|
|
|||
There was a problem hiding this comment.
My local flake8 was complaining about this file, despite me not touching it. 🤷♂️
anoto-moniz
force-pushed
the
feature/pne-6367-feature-effects
branch
from
1d2b4cb to
8a16228
Compare
The payload comes in as a condensed format, so it's expanded in order to constructed nested lists of objects for clarity and ease of use. Additionally, the hierarchy of data is flipped to more closely match how it will be used by our customers. To that end, 'as_dict' is provided to ease importing it into a pandas DataFrame for whatever processing and analysis the customer desires.
anoto-moniz
force-pushed
the
feature/pne-6367-feature-effects
branch
from
8a16228 to
623a307
Compare
kroenlein
reviewed
Dec 4, 2024
Comment on lines
+73
to
+76
| @property | ||
| def as_dict(self) -> Dict[str, Dict[str, Dict[UUID, float]]]: | ||
| """Presents the feature effects as a dictionary by output.""" | ||
| return {output.output: output.feature_dict for output in self.outputs} |
There was a problem hiding this comment.
resource.py supports an as_dict method that is not a property. Does it need to be a property? I'm concerned about changing the signature on a derived class.
There was a problem hiding this comment.
Resource doesn't define as_dict, just GEMDResource.
There was a problem hiding this comment.
Yes, you're right. Not a sig collision.
Choice of property or method doesn't seem to have a consistent flavor in the library. This feels more like a method to me than a property, but acceptable as is if you feel strongly to the contrary.
kroenlein
approved these changes
Dec 11, 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 payload comes in as a condensed format, so it's expanded in order to contructed nested lists of objects for clarity and ease of use.
Additionally, the hierarchy of data is flipped to more closely match how it will be used by our customers. To that end, 'as_dict' is provided to ease importing it into a pandas DataFrame for whatever processing and analysis the customer desires.
PR Type:
Adherence to team decisions