-
Notifications
You must be signed in to change notification settings - Fork 76
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
use correct spectral density equivalencies for marker unit changes #3219
Conversation
Thanks. Is it possible to add regression test that fails on main but passes here? |
(i think this will be fixed by my related ticket btw) |
Does that mean this PR is a duplicate and unnecessary? |
yes but i think its fine as a patch |
@cshanahan1 , so is your plan to revert this in your PR, or undo your patch over there? |
it will just be a one line swap, and test coverage |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3219 +/- ##
=======================================
Coverage 88.56% 88.56%
=======================================
Files 125 125
Lines 18755 18755
=======================================
Hits 16610 16610
Misses 2145 2145 ☔ View full report in Codecov by Sentry. |
I added some test coverage that addresess changes to the flux unit. I wrote additional coverage for translations in Cubeviz that I think should go in a small follow up after @cshanahan1 PR. This is the test on main with the example notebook and the values for the spectrum slice flux and the marker flux: |
jdaviz/configs/default/plugins/markers/tests/test_markers_plugin.py
Outdated
Show resolved
Hide resolved
bf38e73
to
9d67b5d
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.
Thanks!
Description
This pull request is to address updating the spectral density equivalencies to use the original spectral axis so plugin Markers update to the correct position on the spectrum after a unit change.
Original Behavior:
Untitled3.mov
Updated Behavior:
Untitled.2.mov
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.