-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Multi-objective ensemble API #1485
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
aa6e47a
Multi-objective ensemble API
mfeurer 7488955
update for rebase, add loading of X_data in ensemble builder
mfeurer b01f1cb
Add unit tests
mfeurer d8a863c
Fix unittest?, increase coverage (hopefully)
mfeurer 0ba05e9
Rename methods to be Pareto set methods
mfeurer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It's not very clear why this should be only one scikit learn ensemble expected but I assume it's because of the default parameter for ensemble selection.
It also seems this test is very specific to this single case (fitted multiobjective iris classifier).
I had the same problem when considering cases and my solution was just to have general tests. We can just push this through for now, knowing it will break if we add any other cases with the
"multiobjective"
tag.The longer term solution to this, I have a few ideas:
make_automl
,make_dataset
and construct the specific automl instance for this test where the specifics that are being tested are directly evident in the test. Same as old way of doing things and leads to no caching but at least all relevant setup assumptions are stated clearly in test.Happy to hear any other ideas on this though, I admit the caching solution as is, is not perfect for this reason, but it does allow the tests to be a lot more modular.
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.
Correct.
Correct as well.
Very glad you see it this way.
Would we for the 2nd idea check whether the AutoML was built on iris and then use it? Besides that, could we maybe add a filter on which dataset(s) were used to build the AutoML system?
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.
Yup it's definitely possible, the easiest way is to just do so in the test itself, i.e.
if extra_info["dataset"] != "iris": pass
but I'm not the biggest fan of the solution.The overarching problem is that you can't use
@parametrize
and@tags
together, i.e. you can't associate a parameter with a tag.I guess my prefered solution is to include more general things in the
extra_info
or encode it on the model, meaning the tests don't have to do any filtering.It's not the cleanest but at least it means this test could theoretically work for any other "multiobjective" tagged case, as long as it provides the necessary extra_info.