-
Notifications
You must be signed in to change notification settings - Fork 79
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
Change default collapse function to sum #1229
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1229 +/- ##
=======================================
Coverage 77.99% 78.00%
=======================================
Files 90 90
Lines 7185 7187 +2
=======================================
+ Hits 5604 5606 +2
Misses 1581 1581
Continue to review full report at Codecov.
|
self.state.function = 'sum' | ||
self.state.reset_limits() |
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 reset_limits
won't be needed with glue-viz/glue#2277
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.
Would still be needed for older glue even if the upstream PR is accepted?
5431527
to
f7d3bae
Compare
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.
Does what it says it does - changing from sum, deselecting, the reselecting data re-defaults to sum, but I think that is the behavior we want. Might be nice to consider changing the default upstream in glue, but I think this is a good solution for now!
CHANGES.rst
Outdated
- Change default collapse function to sum. [#1229] | ||
|
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.
Is this cubeviz-specific (does it belong under the cubeviz header)?
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.
Yeah, what else is using collapse besides Cubeviz?
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 checked -- Just Cubeviz is using Collapse, though changing the default might have other side effects. If we accept them, we need to update the change log accordingly.
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.
It's not an API change, bug fix, or new feature as far as I could tell, so I put it in the "other" category. Since the change happens in the Specviz viewer file but is most prominent in Cubeviz, I'm not entirely sure where it goes!
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 believe so, although @larrybradley was the original reporter of this ticket. Is that the correct behavior, Larry? |
Yes, the default collapse on load should be sum. I reported on behalf of @PatrickOgle. |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
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.
Thanks!
Description
This pull request changes the default collapse function in the Specviz viewers to sum, instead of maximum.
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.CHANGES.rst
?