-
-
Notifications
You must be signed in to change notification settings - Fork 256
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
Gather CV Results as Completed #433
Gather CV Results as Completed #433
Conversation
The CI failures can probably be ignored. I'll try to get them fixed next week. @vecchp can you think of a way to test this? The best I've come up with is a custom estimator class, that when given a specific hyper parameter combination does something like
@jcrist will you have time next week to give feedback on this approach? |
@TomAugspurger I'll think a little about the tests, though your current suggestion seems reasonable. |
… there a better way?
@jcrist @TomAugspurger made some changes w.r.t the PR comments. On my end all tests pass except for
I also haven't got around to confirming if the futures no longer need to be canceled or writing robust tests for the new as_completed feature. |
@TomAugspurger checking in again to see what else should be done. All tests pass. |
Sorry, I won't have time to look at this for at least the next couple weeks.
…On Thu, Apr 18, 2019 at 12:00 PM Paul Vecchio ***@***.***> wrote:
@TomAugspurger <https://github.com/TomAugspurger> checking in again to
see what else should be done. All tests pass.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#433 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOITO6OQG7S65S3PORK3PRCSLFANCNFSM4GHTECOQ>
.
|
It seems like coverage is decreasing. Does that make sense, or is this a false positive? https://codecov.io/gh/dask/dask-ml/compare/cfa57e5789fc29bdcee5d846b5116c07f5fbe292...9da5b8f0cfa5960a7ee70f216a684bff96a450b2/changes |
Makes sense, looks like it's dead code given the as_completed implementation no longer uses it. I'll update. |
…results_as_completed
@TomAugspurger looks like all stages pass. I think codecov was removed but that should be back to normal as well given the dead code is now gone. |
dask_ml/model_selection/_search.py
Outdated
try: | ||
from cytoolz import get, pluck | ||
except ImportError: # pragma: no cover | ||
from toolz import get, pluck | ||
|
||
try: | ||
from dask.distributed import as_completed |
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.
Is this try/except needed? distributed
is a dependency now (#466).
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'm fine with removing the optional check. Previous guidance was to make it option as it wasn't a dependency at the time.
@stsievert all optional distributed checks in this PR should be removed now. @TomAugspurger any guidance left to get this merged? |
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.
Thanks @vecchp, and apologies once again for the delay on reviewing.
This looks quite good. My only request is a small compatibility layer for build_graph
.
return fit_params | ||
|
||
|
||
def build_cv_graph( |
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.
Can we provide a compatibility shim with the old name? I believe that TPOT is using build_graph
https://github.com/EpistasisLab/tpot/blob/b626271e6b5896a73fb9d7d29bebc7aa9100772e/tpot/gp_deap.py#L429
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 just add back build_graph
with a comment that it's going to be removed (not even a deprecation warning). This is a private module, but I was lazy when adding dask support to TPOT. Once this is in, I can cut a release and make a PR to tpot updating their call.
I'm looking into the compatibility for tpot now. Will push an update. |
Sounds good! |
Thanks @vecchp! |
@TomAugspurger thanks so much for getting this in! |
In progress PR to address #415.