jdaviz
jdaviz copied to clipboard
deconfigged table viewer
Description
This pull request implements a table viewer that can be added in the deconfigged instance and is capable of showing tabular data from the "catalogs" importer (although could be unrestricted to allow any data if we want to allow that) and therefore requires the dev flag to be set: jd.gca().app.state.catalogs_in_dc = True.
Known limitations (likely needing follow-up or fixes upstream in glue-jupyter):
- subset layers do not get styling in the legend/data-menu
- subset columns are not cleared when deleting a subset
- all columns are shown, so a large number of columns causes the table to become unreadable
https://github.com/user-attachments/assets/582dc7ff-c851-4294-85f3-848eeeec5bc0
TODO:
- [x] ~data-menu: restriction to only allow a single entry to be loaded~ (actually seems glue is doing something to merge layers automatically - let's see if it does what we want and if not we can always add restrictions later)
- [x] data-menu: implement/hide visibility toggle
- [x] test coverage
- [x] data-menu to exclude interactive subset-modification options
- [ ] data-menu to include color/styling of subsets? (defer to follow-up: layer state has no connection to color in glue)
- [ ] data-menu to include vis toggle for subsets (defer to follow-up: setting visible has no effect)
- [ ] scope follow-up for choosing which columns are shown - asked @Jenneh for suggestions, could also have horizontal scrolling, but doesn't seem to be supported directly by glue (without just overflowing the whole widget)
Change log entry
- [ ] Is a change log needed? If yes, is it added to
CHANGES.rst? If you want to avoid merge conflicts, list the proposed change log here for review and add toCHANGES.rstbefore merge. If no, maintainer should add ano-changelog-entry-neededlabel.
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.
- [ ] Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the
triviallabel. - [ ] Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
- [ ] Do the proposed changes follow the STScI Style Guides?
- [ ] Are tests added/updated as required? If so, do they follow the STScI Style Guides?
- [ ] Are docs added/updated as required? If so, do they follow the STScI Style Guides?
- [ ] If new remote data is added that uses MAST, is the URI added to the
cache-download.ymlworkflow? - [ ] Did the CI pass? If not, are the failures related?
- [ ] Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
- [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 87.54%. Comparing base (68145d7) to head (db9ae2c).
:warning: Report is 5 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #3867 +/- ##
==========================================
+ Coverage 87.52% 87.54% +0.01%
==========================================
Files 191 192 +1
Lines 26115 26156 +41
==========================================
+ Hits 22856 22897 +41
Misses 3259 3259
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Selecting a row (via checkbox or just clicking somewhere on the row) highlights the entire row and the highlight is persistent making the row hard to read. Selecting a new checkbox moves the highlight and deselecting the checkbox does not clear the highlight. I have a feeling this issue might be upstream...
Yes, let's revisit this when we decide how we want to table to act for more advanced worfklows. Right now this is just using default glue styling/behavior.
Hover text for the column header button that creates subset columns---the 3 blue lines next to index/#---would be very useful! I also initially thought that button meant 'select all'... do we have any way to select all items?
I also thought it was to just sort, but it actually is to create a subset from the selected rows. Agreed that we will either way to ask for a tooltip or create our own if we decide to keep this behavior.
I'm a little confused by the hint Select column to use as source IDs (displayed on mouseover). When I mouseover a row in the column, the text that shows is whatever is in that cell so the only time I get the mouseover to show the source ID is on the ID column. Is the text supposed to be source id anywhere along the row?
Ah, this is from #3856 and controls what goes in mouseover for scatter/image viewers. Maybe we either want to clarify that description or write custom mouseover logic for table viewers eventually?
Ah, this is from #3856 and controls what goes in mouseover for scatter/image viewers. Maybe we either want to clarify that description or write custom mouseover logic for table viewers eventually?
I'd say clarify for now. I could see the utility in having the source ID in mouseover for horizontal scrolling (if that ever gets implemented)/lots of columns but I agree, that's outside the scope of this PR.
Let me know when that's done and I can approve!
@cshanahan1 - any suggestions for changes to the wording to clarify it only applies to mouseover in image/scatter (and not table and I'm guessing histogram viewers)?
@cshanahan1 - any suggestions for changes to the wording to clarify it only applies to mouseover in image/scatter (and not table and I'm guessing histogram viewers)?
Could we simply add "(for image/scatter viewers only)" to the beginning of the description in the loader? It still makes sense to assign it even when loading into a table viewer because you could load it into the app that way, then load it into another viewer type later right?
I'm running into some loading catalogs into the deconfigged app, but they're also present on main so I'll just make follow up tickets for these bugs.
Perfect, thanks!
Is there a way to disable the 'create subset from selected row' button, it looks like a button you would press to select all and since theres no informative text when hovering it's confusing. It would also be nice to have a select all / deselect all button, is that possible?
Possibly, but I think it makes sense to defer that until we scope out a plan for workflows with the table viewer. The intention here is just to pull in the glue table-viewer as-is so that we can build on top of that. If we decide to remove the dev-flag for catalogs before we're ready to make the table viewer public, we can put it behind another dev-flag until we're ready.
Could we simply add "(for image/scatter viewers only)" to the beginning of the description in the loader? It still makes sense to assign it even when loading into a table viewer because you could load it into the app that way, then load it into another viewer type later right?
Works for me, I'll get that included in a few minutes.