-
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
Fix None Row Detection #1798
Fix None Row Detection #1798
Conversation
@pllim do you think this hits the |
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.
good find, thanks!
8562255
to
e651292
Compare
Codecov ReportBase: 87.89% // Head: 87.89% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1798 +/- ##
=======================================
Coverage 87.89% 87.89%
=======================================
Files 95 95
Lines 10186 10186
=======================================
Hits 8953 8953
Misses 1233 1233 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
#1799 will hopefully fix dev test failure. |
e651292
to
cafd4f6
Compare
Weird, the bot didn't do anything? @meeseeksdev backport to v3.1.x |
Thanks for kicking that off Pey-Lian! I was wondering why I never saw any actions from the bot |
Maybe the signal never reached the server or maybe multiple merges around the same time caused some race condition. We'll never know... 🤷 |
…8-on-v3.1.x Backport PR #1798 on branch v3.1.x (Fix None Row Detection)
Description
Found in the
MosvizNIRISSExample
notebook: when the row isn't set properly the visibility filters seem to misbehave. This PR fixes the logic to detect when rows don't have a row assigned, which properly hides them under the data dropdown.Before/After
Looks like the NIRISS parser only assigns rows for the 2D spectra, resulting in this bug appearing:
jdaviz/jdaviz/configs/mosviz/plugins/parsers.py
Line 760 in 8b136cc
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.