-
Notifications
You must be signed in to change notification settings - Fork 101
feat(skore): Add data_source="all" option to EstimatorReport.metrics.summarize
#1907
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(skore): Add data_source="all" option to EstimatorReport.metrics.summarize
#1907
Conversation
|
Caution Some commits in the pull request are not signed, or GitHub is not able to verify the signature. |
|
missing :
|
|
@MatthieuLoustaunau, do you want to finish this PR, or should we take it from here? |
|
I plan to come back at this during the next week, as I will have some spare time. If I cannot finish within 2 weeks I let you take over. |
ed39f5c to
484ad6f
Compare
MarieSacksick
left a comment
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.
Some doctests are failing, could you have a look at why please? It looks like it comes from the new docstring.
Also, could you keep only one summarize function please, and extend the public one (summarize), instead of creating a new private (_summarize) one? Thanks!
|
@MarieSacksick I have removed the |
MarieSacksick
left a comment
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.
There is a doctest that does not pass, could you fix it please? You can run it locally by doing: cd skore && pytest.
And also, could you add a test please? (in the folder /skore/skore/tests/unit/displays/metrics_summary/test_estimator.py)
data_source="all" option to EstimatorReport.metrics.summarize
MarieSacksick
left a comment
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 for me, thanks @MatthieuLoustaunau and @auguste-probabl!
Closes #1446