-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add a _update_multi_column_transformer method #758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, mostly minor comments!
} | ||
|
||
@pytest.mark.parametrize('method_name', methods_to_inputs.keys()) | ||
def test_unvalid_multi_column(self, method_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks, done in e6083f7
|
||
@pytest.mark.parametrize('method_name', methods_to_inputs.keys()) | ||
def test_unvalid_multi_column(self, method_name): | ||
"""Test that the Hypertransformer handles invalid multi column transformers.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be more specific here and add a couple sentences after saying that this tests the case where the update/remove call leaves the transformer as invalid
expected_dict_config['transformers'] = { | ||
'A': UniformEncoder(), | ||
'E': UniformEncoder(), | ||
'C': UniformEncoder(), | ||
'B': UniformEncoder(), | ||
'D': UniformEncoder() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could parametrize this part as well. Basically instead of method_to_inputs being a dict, make it a list of tuples where each tuple has the structure (method_name, input, expected)
, and then just use those variables here accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, thanks, done in e6083f7
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #758 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 2007 2030 +23
=========================================
+ Hits 2007 2030 +23 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CU-86az7g4wc
Resolve #757