feat: Design of EstimatorReport
#997
Conversation
| @@ -1,9 +1,11 @@ | |||
| """Enhance `sklearn` functions.""" | |||
|
|
|||
| from skore.sklearn._estimator import EstimatorReport | |||
There was a problem hiding this comment.
It's disturbing that you want to expose something from a private/protected module.
Shouldn't skore.sklearn.estimator be exposed too by removing _?
There was a problem hiding this comment.
Basically, I want the user to be able to do
skore.EstimatorReportor
skore.sklean.EstimatorReportbut I don't want to expose in a lower level. In scikit-learn (and other package), whenever you don't want people to import from the private module, you add an _ even if it is a folder.
For instance, I would probably to the same for cross_validation.
However, it is something that we can discuss later.
skore/tests/conftest.py
Outdated
| """Setup and teardown fixture for matplotlib. | ||
|
|
||
| This fixture checks if we can import matplotlib. If not, the tests will be | ||
| skipped. Otherwise, we close the figures before and after running the |
There was a problem hiding this comment.
Fmi, why closing before, not just after?
There was a problem hiding this comment.
I don't have a definitive answer since I did not write in scikit-learn. What I can infer is that some test might fail and might not end in the teardown maybe. So the subsequent test is here to make a clean start. However, I'm unsure.
| "estimator[/bold cyan]" | ||
| ) | ||
|
|
||
| def _create_help_tree(self): |
There was a problem hiding this comment.
Can you please add to the helper the representation of the attributes of the reporter.
For instance, it can help users to know that the reporter contains the fitted estimator.
There was a problem hiding this comment.
I added an ending branch listing all getter and init attributes.
| ) | ||
| ) | ||
| # trigger the computation | ||
| list( |
There was a problem hiding this comment.
Maybe we could have a list of indeterminated progress instead of one progress bar that "jumps".
There was a problem hiding this comment.
Happy to see what we can do to improve the current state.
| @@ -0,0 +1,168 @@ | |||
| from typing import Any, Callable, Literal, Optional, Union | |||
There was a problem hiding this comment.
To-do: check if removing the stub files breaks the auto-completion or not, and check if a work-around exists (ping @augustebaum).
Co-authored-by: Sylvain Combettes <[email protected]>
Co-authored-by: Sylvain Combettes <[email protected]>
Co-authored-by: Sylvain Combettes <[email protected]>
Coverage Report for backend
|
|
OK. It should be good to go and we should be able to iterate. |
There was a problem hiding this comment.
Many thanks for this very useful PR @glemaitre and the whole team for reviewing it! Let's iterate on sub-issues if needed
closes #834
Investigate an API for a
EstimatorReport.TODO
__repr____repr__for those displaysEstimatorReport. It could be a bit tricky because they are only defined once the instance created.series.rstpage from pandas to see how they document this sort of pattern.report.metrics.->tabit should provide the autocompetion. edit: having a stub file is actually working. I prefer this than type hints directly in the file.Notes
This PR build upon:
skore.console