-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add vmin and vmax lines to histogram #2301
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2301 +/- ##
==========================================
+ Coverage 90.95% 90.99% +0.03%
==========================================
Files 152 152
Lines 17198 17274 +76
==========================================
+ Hits 15642 15718 +76
Misses 1556 1556
☔ View full report in Codecov by Sentry. |
self._remove_histogram_marks() | ||
self._add_histogram_marks() |
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.
can we just modify the position of the existing marks instead of destroying and creating new ones?
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.
We decided to defer optimizing updating the mark until the effort to use the plot subcomponent (#2254).
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.
I was promised a 30 line code review, this is over double what I was promised! 😉
I find the histogram marks to be quite responsive and the code straightforward to read. Thanks!
(Probably worth a squash & merge once the change log is added) |
Description
This pull request is to address point 4 of this comment #2097 (comment)
Before

After

EDIT: Removed original image, here is the latest:
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Add vmin and vmax lines to plot options histogram. [2301]
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.