Repository navigation
Add Undersampler Component - #2030
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2030 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 284 288 +4
Lines 23271 23425 +154
=========================================
+ Hits 23261 23415 +154
Misses 10 10
Continue to review full report at Codecov.
|
| if previous_component is not None: | ||
| component_dict[component_name].append(f"{previous_component}.x") | ||
| if "sampler" in previous_component: | ||
| component_dict[component_name].extend([f"{previous_component}.x", f"{previous_component}.y"]) |
There was a problem hiding this comment.
Need to track both X and y outputs from the undersampler (using 'sampler' to prepare for potential future oversampler transformers)
chukarsten
left a comment
chukarsten
left a comment
There was a problem hiding this comment.
Bryan, solid work. It's nice to see the base class of the Undersampler get put to work. A few things I'd like to resolve before merge would be:
- What to do about transform()? Can we just relegate the transform() task to the base class?
- The change in the transform() API with the return values. I think we should do our best to expect all component's transform() to return the same number of values and for each return value to have the same type. But I'd like to know other people's thoughts, too.
- The test added in test_component_graph, should it be more generalized? Or should it specifically reference the undersampler.
I think a lot of these can be addressed in a post-standup discussion! Still, overall great work. Definitely appreciate the clarity of the tests.
| train_indices = index_df[index_df.isin(indices)].dropna().index.values.tolist() | ||
| return X.iloc[train_indices], y.iloc[train_indices] | ||
|
|
||
| def transform(self, X, y): |
There was a problem hiding this comment.
Is this necessary? I need to check if this is how we handle other components that don't need transform. Is there any ambiguity in someone calling .transform() on their data and perhaps expecting the sampling to be applied?
There was a problem hiding this comment.
In our base Transformer class, we end up trying to transform with self._component_obj, which would raise an error since we don't have a transform method with the undersampler. I overrode it to avoid this issue.
There was a problem hiding this comment.
Hmmm, this is curious. Does it make more sense to raise a NotImplementedError?
There was a problem hiding this comment.
Hmm, I don't think so. When we try to score/predict on a pipeline, we call transform() on all of the transformer components rather than fit_transform, which makes sense since we don't want to refit the component if we have already trained the pipeline and are predicting. In this scenario, our sampler shouldn't do anything (since we only sample during training), and so we should allow users to both call transform() and get a result from it.
There was a problem hiding this comment.
I think this is a cool way of specifying "train only" transformers! I was thinking we'd have to pass info from the pipeline down to the component but I think it's great we can do something simpler.
The one confusing thing is that fit().transform() != fit_transform() which I think is what someone coming fresh off sklearn would expect. Maybe we should just be super clear about which transformers are "train only" and what users should expect with them. I think "train only" transformers are a cool concept we can use to differentiate ourselves.
There was a problem hiding this comment.
Yeah, that's a good point! Would it be best to state that this is a 'train only' transformer in the documentation/description of the component?
| assert component_graph.get_parents('OneHot') == ['Imputer.x'] | ||
| assert component_graph.get_parents('Undersampler') == ['OneHot.x'] | ||
| assert component_graph.get_parents('Random Forest') == ['Undersampler.x', 'Undersampler.y'] | ||
| assert component_graph.get_parents('Elastic Net') == ['Undersampler.x', 'Undersampler.y'] |
There was a problem hiding this comment.
Interesting, I think I'm a little out of the loop with respect to the component graphs. Is it expected that there's a branch from the undersampler to elastic net and rf? i assume because theyre both estimators that makes both estimators children of the last component?
That kind of leads me to another question : is this a test specific to the undersampler? Or would the test look like this without the undersampler?
assert component_graph.get_parents('Imputer') == []
assert component_graph.get_parents('OneHot') == ['Imputer.x']
assert component_graph.get_parents('Random Forest') == ['OneHot.x', 'OneHot.y']
assert component_graph.get_parents('Elastic Net') == ['OneHot.x', 'OneHot.y']
If it would look like the above with OneHot taking the place of Undersampler, I think we really just need a more generalized test that tests the behavior with respect to the transformer/estimator boundary in the component graph without necessarily using the Undersampler.
There was a problem hiding this comment.
@chukarsten, currently, our component parents for all previous transformers only stores the x value in the component_graph parents' rather than storing both X and y since our previous transformers didn't alter y. I added in the support for y since our sampling methods (both over and under) will all be transforming both X and y, which is why the tests I added were specific to the undersampler. For your example, it would look like:
assert component_graph.get_parents('Imputer') == []
assert component_graph.get_parents('OneHot') == ['Imputer.x']
assert component_graph.get_parents('Random Forest') == ['OneHot.x']
assert component_graph.get_parents('Elastic Net') == ['OneHot.x']There was a problem hiding this comment.
So is this test a test that it's handling the passing forward of the y value?
There was a problem hiding this comment.
yep, this is making sure that when we add in an undersampler component to the pipeline, we keep track of both the X and y output of that component, while for other components we still only track the X output. This is definitely something that we can discuss in conjunction with your point about having the output of transform() be normalized among all transformers.
…valml into bc_2020_undersampler_component
| ) | ||
|
|
||
|
|
||
| class BaseSampler(Transformer): |
There was a problem hiding this comment.
Added this base class to expand all of our future under/oversamplers with.
There was a problem hiding this comment.
I think this is a solid add and we'll reap the rewards of it later.
| assert input_feature_names['Logistic Regression'] == ['Random Forest', 'Elastic Net'] | ||
|
|
||
|
|
||
| def test_component_graph_sampler(): |
There was a problem hiding this comment.
Was thinking about this RE target imputer, but can you try adding a test for fitting and predicting with a component graph that has an undersampler and confirm that it works?
There was a problem hiding this comment.
Added a test in the pipeline tests!
6 participants
fix #2020