134 remove returnkeydict class #135
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 need for the ReturnKeyDict class for handling unmapped values.
decouple from BaseMappingTransfromMixin.
…sformer docsting entry for mappings attribute
TommyMatthews
approved these changes
Dec 14, 2023
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.
ReturnKeyDict was used in the past to facilitate leaving unmapped values in the target dataframe unchanged. Pandas also has a Series.replace() method which can achieve this functionality without the need for the ReturnKeyDict class.
This also means we can stop looping over the columns to map in BaseMappingTransformerMixin.transform() as the replace method natively supports reading a dictionary with the first level defining columns to map.
The behaviour of mapping required for MeanResponseTransformer was not preserved with this change. However I have decoupled it from BaseMappingTransformerMixin and implemented MeanResponseTransformer.map_imputer_values() instead since the feature of leaving unmapped values alone is not required here and thus the coupling was unhelpful.
In going through this process I have come to the conclusion that MeanResponseTransformer could do with a refactor!