Add __eq__ for ComponentBase+PipelineBase
#1178
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1178 +/- ##
==========================================
+ Coverage 99.73% 99.92% +0.18%
==========================================
Files 196 196
Lines 11813 11977 +164
==========================================
+ Hits 11782 11968 +186
+ Misses 31 9 -22
Continue to review full report at Codecov.
|
angela97lin
requested review from
dsherry,
freddyaboulton,
jeremyliweishih and
bchen1116
There was a problem hiding this comment.
@angela97lin LGTM! My only blocking comment was about not checking the class attributes during comparison, only instance attributes.
There was a problem hiding this comment.
@angela97lin I think this is great! I think the tests are pretty extensive and the implementation is cool. My only comment is that I think it would be good to write a test to check __eq__ for all currently and future defined components using pytest parametrize and all_components(). This would automatically test all components as they're added which would help us catch breaking changes as they're added.
| @@ -781,3 +781,62 @@ def test_estimators_accept_all_kwargs(estimator_class): | |||
| # Deleting because we call it random_state in our api | |||
| del params["random_seed"] | |||
| estimator_class(**params) | |||
|
|
|||
There was a problem hiding this comment.
I think it would be good to check that all of our components are equal with themselves and not equal with the same component class with different parameters just to safeguard against future refactorings of our api and automatically check that new parameters don't break __eq__
@pytest.mark.parametrize("component_class", all_components())
def test_equality_all_defined_components(component_class):
assert component_class() == component_class()
kwargs = {"foo": 1, "bar": 2}
assert component_class(**kwargs) != component_class()There was a problem hiding this comment.
Oo sure, looks like I had to do the same mocking set up in test_components_init_kwargs to pass in kwargs that won't break, so I'm going to add two lines there rather than duplicate the mocking in a separate test. I added the assert component_class() == component_class() tests for all components too.
Closes #475.