Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

edited MeanResponseTransformer to convert category to float, fixed tests #174

Merged
merged 14 commits into from
Feb 6, 2024

Conversation

limlam96
Copy link
Contributor

@limlam96 limlam96 commented Feb 2, 2024

Fixed issue where category dtypes were preserved by this transformer, which is not desired behaviour. Edited to coerce outputs to float, as converting to numeric is ultimately the goal of this one.

Fixed tests, and edited some to put a category column through as part of testing.

Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @limlam96! Idea and implementation look sound. This transformer is already a bit overcomplicated resulting in some difficult to interpret code and tests so most of my comments relate to avoiding making tests harder to read.

tubular/nominal.py Show resolved Hide resolved
tests/nominal/test_MeanResponseTransformer.py Outdated Show resolved Hide resolved
tests/nominal/test_MeanResponseTransformer.py Show resolved Hide resolved
tests/nominal/test_MeanResponseTransformer.py Outdated Show resolved Hide resolved
tests/nominal/test_MeanResponseTransformer.py Outdated Show resolved Hide resolved
tests/nominal/test_MeanResponseTransformer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with changes, thanks!

@davidhopkinson26 davidhopkinson26 merged commit 81003b9 into main Feb 6, 2024
1 check passed
@davidhopkinson26 davidhopkinson26 deleted the feature/mean_response_encoder_coerce_float branch February 6, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants