-
Notifications
You must be signed in to change notification settings - Fork 73
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
markers: ensure units stored as string #3135
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
=======================================
Coverage 88.82% 88.82%
=======================================
Files 112 112
Lines 17430 17430
=======================================
Hits 15482 15482
Misses 1948 1948 ☔ View full report in Codecov by Sentry. |
So, is it |
at this point I just want to ensure that what is stored is a string, is casting not ok for that, do we need to check the type and call |
Well, what would it be if not a string nor Unit? It can't suddenly be a number, can it? I think you can cast |
Exactly. Since I don't need to override the formatting, I thought |
Can't tell what is better. I guess this works for now if you need something that can cover other data types as well. If a day comes when we must roundtrip back to FITS format or something, we can reconsider. |
If this is to fix a bug, do we need a new regression test? |
Working on it - it isn't as easy as it might seem because the existing markers test uses a cube in Jy, which does not show this bug, and switching it to the Jy/sr test fixture changes all the other asserts, so I will need to confirm that those are all correct. That said - its hard to cover every possible combination and order of adding markers that could result in any conflicting units, so we may have to just live with line coverage to some extent. |
Since adding tests are not easy and the diff looks uncontroversial, I think we can proceed without tests. Thanks! |
Description
This pull request ensures the units in the markers plugin are in string and fixes adding a marker to the spectrum viewer followed by one of the cube viewers. This bug is confirmed to not be present in 3.10.x, so should not need backporting or a changelog entry.
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.