-
Notifications
You must be signed in to change notification settings - Fork 75
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
BUG: Handle incompatible Specviz flux unit #2485
Conversation
c8f80e3
to
bff7eae
Compare
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
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.
Just a couple comments below but otherwise this approach seems fine to me
8625317
to
f25dae2
Compare
Thanks for the review, @astrofrog ! I applied your suggestions and added you as co-author for the commit. |
Co-authored-by: Thomas Robitaille <thomas.robitaille@gmail.com>
f25dae2
to
67b7cc5
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.
It's good we no longer silently ignore data with incompatible flux units and make a clear snackbar message that it was not loaded. This seems like the best solution while we work on finding a way to work with /sr
units.
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.
Looks good other than the edit I suggested for the snackbar text. Feel free to merge after accepting that (or rejecting if you feel strongly about it!).
…5-on-v3.7.x Backport PR #2485 on branch v3.7.x (BUG: Handle incompatible Specviz flux unit)
Description
This pull request is to see if we can handle spectrum with incompatible flux unit without having to introduce more links.
While I was at it, I also try the "hold sync" trick on a couple of plotting code, like Maarten said. Technically irrelevant to the bug but doesn't hurt?
Fixes #2459
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.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.