-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(cross-validation): Split time-related results into their own plots #986
base: main
Are you sure you want to change the base?
Conversation
0fe1e91
to
6b0603e
Compare
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.
Nice work, just a few feedbacks.
def linspace(lo, hi, num): | ||
interval = (hi - lo) / (num - 1) | ||
return [lo + k * interval for k in range(0, num)] |
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 use numpy.linspace
here ?
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 did originally, but re-implementing it avoids bringing in numpy as a direct dependency for just 1 function
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.
...although I see that we import numpy in cross_validation_item.py
...
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.
And numpy is a direct dependency of sklearn which is our direct dependency. ♻️
skore/src/skore/sklearn/cross_validation/plots/compare_scores_plot.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/cross_validation/plots/timing_normalized_plot.py
Outdated
Show resolved
Hide resolved
def linspace(lo, hi, num): | ||
interval = (hi - lo) / (num - 1) | ||
return [lo + k * interval for k in range(0, num)] |
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.
Same as above could you use numpy.linspace
?
# FIXME: Maybe this logic belongs in CrossValidationPlots | ||
plots_bytes = { | ||
plot_name: plotly.io.to_json(plot, engine="json").encode("utf-8") | ||
for plot_name, plot in dataclasses.asdict(plots).items() |
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.
Could the plot name be "humanized" ?
timing_normalized
=> Normalized timings ?
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.
You mean just in the front-end right? I mean, in their Python code user would still have to write
reporter.plots.timing_normalized
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.
How should the other plots be renamed? Maybe something like
- compare_scores -> "Scores"
- timing -> "Timings"
- normalized_timing -> "Normalized timings"
@MarieS-WiMLDS Does this make sense?
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.
Yes only frontend side !
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 completely makes sense! I like that it's easy from the human name to find how to call it in python. With that logic, maybe we could even switch from compare_scores
to just scores
?
Closes #902
Screen-record:
2024-12-18T15_00_29_screen_record.mp4
plots
attributeplot_cross_validation
function