-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Set rescale_discrete_levels=True by default #5268
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5268 +/- ##
==========================================
- Coverage 86.74% 86.62% -0.12%
==========================================
Files 299 299
Lines 62218 62281 +63
==========================================
- Hits 53969 53950 -19
- Misses 8249 8331 +82
Continue to review full report at Codecov.
|
@jbednar should this also be merged before 1.15.0? |
Yes |
1 similar comment
Yes |
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Bokeh 2.4.3 EqHistColorMapper now supports |
Already seems to be true in the bokeh case, but really we need to implement it for matplotlib. |
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.
The MPL changes look good, thanks!
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
The
eq_hist
colormapper used with Datashader is designed for large datasets with a wide distribution of values, but as documented in holoviz/datashader#357 the behavior when there are only a few distinct value levels has always been confusing and misleading. Datashader 0.14 adds an optionrescale_discrete_levels=True
, now supported in HoloViews, allowing users to avoid this issue. However, this option is False by default, and most users are never going to be aware that the option exists. This PR enables the option by default, which has good and bad consequences:eq_hist
that have only a small number of discrete levels. That's by design, but it is strictly a compatibility change, which is odd to introduce in a patch release.rasterize
anddatashade
, because Bokeh does not yet supportrescale_discrete_levels
. So there is some argument for waiting until Bokeh does support it to make this the default.Basically, if we consider this a bugfix, we should introduce it now; patch releases are for bugfixes. If we consider it a new feature, we should wait until the next major or at least minor release to enable it by default.
I'm ok with either option, as long as the options have been duly considered and decided between.