Dont allow duplicate pipeline names in AutoMLSearch#1932
Conversation
freddyaboulton
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1932 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 267 267
Lines 21700 21730 +30
=========================================
+ Hits 21694 21724 +30
Misses 6 6
Continue to review full report at Codecov.
|
| pipeline.threshold = objective.optimize_threshold(y_predict_proba, y_threshold_tuning, X=X_threshold_tuning) | ||
|
|
||
|
|
||
| def check_all_pipeline_names_unique(pipelines): |
There was a problem hiding this comment.
classic stacked branches. disappointed this was only one on another and not 5
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
LGTM! 🥳
If we want to be super sure maybe it'd be good to add test cases for when multiple (>3) pipelines have the same name to make sure its printed out once (Custom, Custom, Custom), or multiple pipelines with the same names (Custom, Custom, Custom1, Custom1) but probably not super necessary since at that point we're just testing set functionality 😂
|
|
||
| with pytest.raises(ValueError, | ||
| match="All pipeline names must be unique. The names 'Custom Pipeline' were repeated."): | ||
| AutoMLSearch(X, y, problem_type="binary", allowed_pipelines=[MyPipeline1, MyPipeline2, MyPipeline3]) |
There was a problem hiding this comment.
Super duper nit-pick suggestion but I wonder if there's a way to phrase this so that it gramatically makes sense for the case with 1 or multiple duplicates :P
There was a problem hiding this comment.
I should've known @angela97lin was driving the tense calculator
There was a problem hiding this comment.
I had to heavily debate whether it was worth it or not to comment about this LOL 😅
jeremyliweishih
left a comment
jeremyliweishih
left a comment
There was a problem hiding this comment.
looks good but I like Angela's suggestion on fixing the grammar in the one duplicate case!
chukarsten
left a comment
chukarsten
left a comment
There was a problem hiding this comment.
Proposed alternate impl for the check to reduce lines, but no need to accept. Great work.
| pipeline.threshold = objective.optimize_threshold(y_predict_proba, y_threshold_tuning, X=X_threshold_tuning) | ||
|
|
||
|
|
||
| def check_all_pipeline_names_unique(pipelines): |
There was a problem hiding this comment.
classic stacked branches. disappointed this was only one on another and not 5
| None | ||
|
|
||
| Raises: | ||
| ValueError if any pipeline names are duplicated. |
There was a problem hiding this comment.
I think I mentioned in your other PR that normally I think we do: ValueError: if....
| seen_names.add(pipeline.name) | ||
|
|
||
| if duplicate_names: | ||
| plural, tense = ("s", "were") if len(duplicate_names) > 1 else ("", "was") |
There was a problem hiding this comment.
I respect this dedication to verb tense calculation.
| seen_names = set() | ||
| duplicate_names = set() | ||
|
|
||
| for pipeline in pipelines: | ||
| if pipeline.name in seen_names: | ||
| duplicate_names.add(pipeline.name) | ||
| else: | ||
| seen_names.add(pipeline.name) |
There was a problem hiding this comment.
I have twice proposed a pandas impl and twice deleted it thinking I was being overbearing. Here it is...not blocking, feel free to reject.
name_count = pd.Series([p.name for p in pipelines]).value_counts()
duplicate_names = name_count[name_count > 1] # I don't think this line is quite right.
There was a problem hiding this comment.
Ended up taking it! 👏
|
|
||
| with pytest.raises(ValueError, | ||
| match="All pipeline names must be unique. The names 'Custom Pipeline' were repeated."): | ||
| AutoMLSearch(X, y, problem_type="binary", allowed_pipelines=[MyPipeline1, MyPipeline2, MyPipeline3]) |
There was a problem hiding this comment.
I should've known @angela97lin was driving the tense calculator
4 participants
Pull Request Description
Fixes #1858
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.