-
Notifications
You must be signed in to change notification settings - Fork 45
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
Alternative to #599 with less logging #600
Conversation
re-export computational resources
Codecov Report
@@ Coverage Diff @@
## dev #600 +/- ##
==========================================
+ Coverage 85.15% 85.71% +0.56%
==========================================
Files 37 37
Lines 3192 3269 +77
==========================================
+ Hits 2718 2802 +84
+ Misses 474 467 -7
Continue to review full report at Codecov.
|
@CameronBieganek Assuming you're pretty busy. But in case this fell off your radar... 🙏🏾 |
@ablaom Yeah, I've been pretty tied up lately. And I'm going on a short trip Sunday through Wednesday. But I would definitely like to help—I'll try to squeeze this in before I leave. 😃 |
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.
Looks pretty good overall. I made a few minor comments here and there.
Are all those verbosity=0
in the unit tests still needed with the new version of this PR?
src/resampling.jl
Outdated
@@ -465,12 +501,18 @@ function _check_measure(measure, model, y, operation) | |||
if model isa Probabilistic | |||
if operation == predict | |||
if prediction_type(measure) != :probabilistic |
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.
Maybe you can reduce the nesting of the if
blocks by writing
if (model isa Probabilistic) && (operation == predict) && (prediction_type(measure) != :probabilistic)
I'm guessing you wrote it your way for readability or to stay within the line length limit, but I think the logic is easier to understand when the conditions are combined into one boolean expression.
Maybe something like this for a readable typesetting:
is_non_probabilistic_measure = (
model isa Probabilistic &&
operation == predict &&
prediction_type(measure) != :probabilistic
)
if is_non_probabilistic_measure
# ...
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.
Good suggestion. Adopted.
(x..., y, z) instead of tuple(x..., y, z) Co-authored-by: Cameron Bieganek <8310743+CameronBieganek@users.noreply.github.com>
Co-authored-by: Cameron Bieganek <8310743+CameronBieganek@users.noreply.github.com>
Co-authored-by: Cameron Bieganek <8310743+CameronBieganek@users.noreply.github.com>
Co-authored-by: Cameron Bieganek <8310743+CameronBieganek@users.noreply.github.com>
@CameronBieganek Many thanks for looking over this! Sorry about |
Continuation of #599, implementing variation suggested at #599 (comment)