Skip to content
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

EstimatorReport repr method has unintended side-effects #1293

Closed
auguste-probabl opened this issue Feb 7, 2025 · 0 comments · Fixed by #1304
Closed

EstimatorReport repr method has unintended side-effects #1293

auguste-probabl opened this issue Feb 7, 2025 · 0 comments · Fixed by #1304
Assignees
Milestone

Comments

@auguste-probabl
Copy link
Contributor

auguste-probabl commented Feb 7, 2025

The __repr__ method of EstimatorReport prints a help message:

>>> report
╭─────── skore.EstimatorReport ────────╮
│ Get guidance using the help() method │
╰──────────────────────────────────────╯

This can cause strange things to happen when printing things containing an EstimatorReport:

>>> [report, report]
[╭─────── skore.EstimatorReport ────────╮
 │ Get guidance using the help() method │
 ╰──────────────────────────────────────╯,
 ╭─────── skore.EstimatorReport ────────╮
 │ Get guidance using the help() method │
 ╰──────────────────────────────────────╯]

In VSCode it looks even less intelligible:

Image

Perhaps a strategy similar to sklearn can be used, where the repr shows an HTML widget when the estimator is shown on its own, but it shows a string when the estimator is inside another object.

Originally reported by @MarieS-WiMLDS

@MarieSacksick MarieSacksick added this to the skore 0.8 milestone Feb 7, 2025
@auguste-probabl auguste-probabl self-assigned this Feb 10, 2025
auguste-probabl added a commit that referenced this issue Feb 10, 2025
…ort`

This makes it more practical to print objects containing such reports,
e.g. `[report]`.

Closes #1293
auguste-probabl added a commit that referenced this issue Feb 10, 2025
…ort`

This makes it more practical to print objects containing such reports,
e.g. `[report]`.

Closes #1293
thomass-dev added a commit that referenced this issue Feb 19, 2025
#1286)

- [x] Rename to `ComparisonReport`
- [x] Rebase on top of #1239 and adapt
- [x] Raise if `report.metrics.accuracy(data_source="train")` is called
with at least one EstimatorReport that does not have training data
- [x] Test
- [x] Docstrings
    - [x] MetricsAccessor
- [x] Move index column "#0" in front of each metric
- [x] Pass report names in comparator
- [ ]  ~Update plots legend~ see #1309 
- The actual `RocCurveDisplay` needs a full refactor to be splitted by
use-case: estimator report, cross-validation report and finally
comparison report. In each of these use-cases, there is two scenarios
with binary classification and multi-class classification. Otherwise, it
will be unmaintainable.
- [ ] ~Investigate missing metrics in `report_metrics`~ **(deferred to
future PR)**
- The logic is split between `report_metrics` and `available_if`; it
should be merged (ideally everything in `available_if`?)
- [ ] ~Refactor to make `CrossValidationReport` depend on it~
**(deferred to future PR)**
- [x] ~Change EstimatorReport `repr`?~ Issue
#1293

Closes #1245 

Co-authored-by: Auguste <[email protected]>
Co-authored-by: Sylvain Combettes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants