Skip to content
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

Display units (unit conversion refactor) #2127

Merged
merged 51 commits into from
Jun 13, 2023
Merged

Display units (unit conversion refactor) #2127

merged 51 commits into from
Jun 13, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Apr 4, 2023

Description

This pull request compiles all the efforts in the unit conversion refactor to support display units and will remain in draft mode until all those individual efforts are merged into the development branch. Until then, this can serve as a thread to track items that need to be done/tested before eventual merge into main.

Incremental PRs:

Bugs that need fixing upstream before merge:

Needs testing:

Other TODOs:

  • any other attributes (meta, wcs, etc) in core.helpers.ConfigHelper._get_data._handle_display_units
  • 2d-spectrum-viewer (specviz2d, mosviz) support or disable plugin (deferring support)
  • image viewer support (in mouseover) for cubeviz (deferring support)
  • consider renaming plugin (and update docs accordingly)

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 to CHANGES.rst before merge. If no, maintainer
    should 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.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • 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?
  • 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.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@kecnry kecnry mentioned this pull request Apr 4, 2023
9 tasks
@kecnry kecnry force-pushed the dev-display-units branch from 63f23d2 to ee8deb9 Compare April 4, 2023 17:43
@kecnry kecnry added this to the 3.5 milestone Apr 5, 2023
@pllim

This comment was marked as resolved.

@kecnry kecnry force-pushed the dev-display-units branch 3 times, most recently from e59b2f9 to c37463e Compare April 19, 2023 00:39
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Patch coverage: 90.73% and project coverage change: +3.19 🎉

Comparison is base (ae6d467) 88.48% compared to head (35dfc58) 91.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2127      +/-   ##
==========================================
+ Coverage   88.48%   91.67%   +3.19%     
==========================================
  Files          95      148      +53     
  Lines       10540    16752    +6212     
==========================================
+ Hits         9326    15357    +6031     
- Misses       1214     1395     +181     
Impacted Files Coverage Δ
...s/imviz/plugins/line_profile_xy/line_profile_xy.py 99.00% <ø> (-0.04%) ⬇️
...onfigs/mosviz/plugins/slit_overlay/slit_overlay.py 88.15% <ø> (+17.10%) ⬆️
jdaviz/configs/specviz/plugins/parsers.py 90.80% <ø> (+0.27%) ⬆️
jdaviz/core/tools.py 83.25% <ø> (+1.48%) ⬆️
jdaviz/core/user_api.py 92.45% <ø> (+4.45%) ⬆️
jdaviz/core/validunits.py 82.14% <ø> (-17.86%) ⬇️
jdaviz/models/physical_models.py 91.74% <ø> (+0.07%) ⬆️
jdaviz/utils.py 90.16% <ø> (+0.08%) ⬆️
jdaviz/configs/imviz/tests/test_parser_roman.py 10.00% <10.00%> (ø)
jdaviz/cli.py 20.89% <22.72%> (-1.33%) ⬇️
... and 86 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pllim and others added 6 commits May 15, 2023 15:51
* basic app-wide display unit in specviz (not all plugins are updated yet)
* unit-aware select component which maps to glue-supported unit strings
* implement use_display_units option for get_data, get_subsets

Co-authored-by: Jesse Averbukh <javerbukh@gmail.com>
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
* emit event when display unit changed
* update line analysis for unit changes
(we might want to generalize some of this logic more into the mark's _update_data when adding support for spectral lines as well)
* update markers plugin for display units
* move unit-updating logic to mark itself
* LineUncertainties to be unit-aware
* fix support for markers in cubeviz
pllim and others added 7 commits June 7, 2023 07:59
disallow gaussian smooth output as input
in app.get_subsets()
BUG: Fix ellipse translation in app.get_subsets()
FEAT: Load annulus from file, load reg files from IMPORT DATA
Added .ico and .svg files
* DOC: Use pydata-sphinx-theme
that has dark mode, modern design, and mobile friendly

DOC: No longer need tomli

DOC: Now requires sphinx-astropy 1.9.1

* Add cards to index page
with icons

* Remove iframe hardcoding so it scales
in mobile mode

* DOC: Insert small logo next to title
instead of a giant logo above the title
* Add app method to simplify spectral subset

Add simplify button to subset plugin

* Fix xor bug exposed by simplify button

* Fix style checks

* Remove print statements

* Add tooltip to simplify button

* Make button appear only if the subset can be simplified

* Make simplify button appear only when applicable

* Apply suggestions from code review

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Update changes file

---------

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, a few things don't work perfectly in line lists but we were going to defer that to a larger overhaul of line lists.

@rosteen rosteen merged commit 19f89e4 into main Jun 13, 2023
rosteen added a commit that referenced this pull request Jun 13, 2023
@pllim pllim deleted the dev-display-units branch June 13, 2023 17:37
kecnry added a commit to kecnry/jdaviz that referenced this pull request Jun 13, 2023
* introduced by spacetelescope#2127: spatial subsets crashed when trying to display the unit in the subset plugin, this now implements a fallback to not show the unit suffix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants