Remove fillna call in ArbitraryImputer (Issue 195) #198
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
…BaseImputer.transform to return a dataframe instead of an int
|
This looks good to me RJ, looks like there are just a couple of ruff things failing in CI - could you fix those and then will approve? :) |
|
Hi Liam, the ruff linting errors seem to be from tests/imputers/test_BaseImputer.py due to using np.NaN rather than np.nan. I have made this replacement now and all the checks are passing :) |
limlam96
approved these changes
Mar 18, 2024
limlam96
requested changes
Mar 18, 2024
|
Done! Have a look at the changelog and let me know if it's fine :) |
limlam96
approved these changes
Mar 18, 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.
ArbitraryImputer was calling fillna both in it's own transform method, and in the BaseImputer method.
To increase tubular's efficiency and maintainability, have removed fillna call in ArbitraryImputer and moved the logic for "casting imputer value as same dtype as original column" below the BaseImputer.transform call.
Note that the above change caused the test_impute_values_set in test_ArbitraryImputer.py to fail since the initial mock setup for BaseImputer.transform was returning a simple integer:1234. However, this causes 'TypeError: 'int' object is not superscriptable' for the following code line in transform method of ArbitraryImputer: X_transformed[c] = X_transformed[c].astype(dtype), since an integer cannot be treated as a collection. Therefore have updated test_impute_values_set to mock BaseImputer.transform to return a dataframe rather than an int