[RFC] Simple __repr__ with global flag#9039
[RFC] Simple __repr__ with global flag#9039amueller wants to merge 13 commits intoscikit-learn:masterfrom
Conversation
|
How about we try merge #7548 and reuse its mechanics for global config... |
|
@jnothman good point |
Codecov Report
@@ Coverage Diff @@
## master #9039 +/- ##
=========================================
Coverage ? 96.19%
=========================================
Files ? 331
Lines ? 59667
Branches ? 0
=========================================
Hits ? 57395
Misses ? 2272
Partials ? 0
Continue to review full report at Codecov.
|
|
To summarize an discussion that I had IRL with @amueller: sometimes hiding the default is a dangerous thing, as the default is not good (for instance because it is impossible to have a uniformly good default). One example is n_clusters, or n_components. Another is n_estimators in RandomForests, which is at 10 right now (and that's way too low). One suggestion is to add to the mechanism in this PR an estimator-specific whitelist of parameters that should always be listed. |
|
hm I'm not as enthusiastic any more and I think maybe I don't want to make this a priority for the sprint, given all the other cool stuff we need to work on. |
|
At least I fixed the pretty printing... comparison of pretty printing, pretty printing only changed parameters, pretty printing and coloring changed parameters, and the current repr. |
|
the current pretty printing can't really work because it hands control to |
|
oh wait above was the simple example, here pretty print, pretty print with changed vs current: |
jnothman
left a comment
There was a problem hiding this comment.
This comment was pending. I'll try look later
| return filtered_params | ||
| return params | ||
|
|
||
| def __repr__(self): |
There was a problem hiding this comment.
I think there may be contexts in which we want to override the global option, such as in naming estimators in check_estimator tests..?
|
A global blacklist including things like n_jobs, verbose and random_state
might be sensible...? Not sure. I certainly take your point Gaël. I do
think it would be nice to have a way to see which params are not default
On 9 Jun 2017 8:02 am, "Andreas Mueller" <notifications@github.com> wrote:
oh wait above was the simple example, here pretty print, pretty print with
changed vs current:
[image: image]
<https://user-images.githubusercontent.com/449558/26951769-b2da9b0e-4ca2-11e7-828b-1def0007fc24.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9039 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6xzEhl6IFet-imllWQbfyJ1bRolVks5sCG96gaJpZM4Nyq2H>
.
|
|
Oh sorry, above comparison was not against master, it was against my hacked master. Here's what it actually looks like:
Marking changed parameters with color kinda failed. For pipeline, do you mark the whole estimator? Or just the estimator name? Or the parameter name (see example above). |
|
The current ```repr`` makes no sense at all. There's an easy fix to the more consistent indentation and there's a larger fix (that I implemented) to actually pretty print by taking charge of dictionary, list and array formatting. |
|
Rather than marking non-defaults, make all defaults grey, name and value.
That shouldn't lead to the same inconsistencies.
On 9 Jun 2017 6:49 pm, "Andreas Mueller" <notifications@github.com> wrote:
The current ```*repr*`` makes no sense at all. There's an easy fix to the
more consistent indentation and there's a larger fix (that I implemented)
to actually pretty print by taking charge of dictionary, list and array
formatting.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9039 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66jWeF62tRfHeH3ixR0yr3QiIUueks5sCQcWgaJpZM4Nyq2H>
.
|
|
@jnothman what do you think about the formatting vs the current formatting? @GaelVaroquaux liked it, might open a PR. |
|
i think the nesting is helpful. not looked at code to see how you might
handle ellipsis
On 10 Jun 2017 9:42 pm, "Andreas Mueller" <notifications@github.com> wrote:
@jnothman <https://github.com/jnothman> what do you think about the
formatting vs the current formatting? @GaelVaroquaux
<https://github.com/gaelvaroquaux> liked it, might open a PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9039 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6yBvk5P-M5m32aea9wK1RkNM_xtpks5sCoEmgaJpZM4Nyq2H>
.
|
|
A note: I believe that there are contexts in which the default parameters should not be shown, such as when printing out parameters (some of which are estimators) being searched. |
|
replaced by #11705 |
Following up on #7618 which attempted a fancy repr for Jupyter, this goes back to changing
__repr__in all cases to only show non-default parameters, configured by a global option.The idea is to document the introduction of the argument, and I think set the configuration for the documentation, then change the default value in 2 versions. Docs a-coming.
ping @jnothman