display units: fix failing tests #2132

kecnry · 2023-04-05T19:16:31Z

Description

This pull request fixes failing tests on CI now that we have glue 1.9.1 pinned.

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 · 2023-04-13T17:37:27Z

jdaviz/configs/specviz/helper.py

@@ -128,18 +128,23 @@ def get_spectra(self, data_label=None, subset_to_apply=None, apply_slider_redshi

            return output_spectra

-    def get_spectral_regions(self):
+    def get_spectral_regions(self, use_display_units=False):


we might want to discuss the default option here and in get_subsets (but can change that anytime before merging the dev unit conversion branch into main)

I can see arguments both ways but being a data purist, I would want the regions to be in data unit, not display unit.

codecov · 2023-04-13T18:06:25Z

Codecov Report

❗ No coverage uploaded for pull request base (dev-display-units@5a33d08). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head a41c92f differs from pull request most recent head 7bda40b. Consider uploading reports for the commit 7bda40b to get more accurate results

Additional details and impacted files

@@                 Coverage Diff                  @@
##             dev-display-units    #2132   +/-   ##
====================================================
  Coverage                     ?   91.97%           
====================================================
  Files                        ?      146           
  Lines                        ?    15958           
  Branches                     ?        0           
====================================================
  Hits                         ?    14678           
  Misses                       ?     1280           
  Partials                     ?        0

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pllim

Just minor comments.

jdaviz/configs/cubeviz/plugins/slice/slice.py

pllim · 2023-04-13T18:41:39Z

jdaviz/configs/specviz/helper.py

@@ -128,18 +128,23 @@ def get_spectra(self, data_label=None, subset_to_apply=None, apply_slider_redshi

            return output_spectra

-    def get_spectral_regions(self):
+    def get_spectral_regions(self, use_display_units=False):


I can see arguments both ways but being a data purist, I would want the regions to be in data unit, not display unit.

jdaviz/configs/specviz/helper.py

pllim · 2023-04-13T18:43:18Z

jdaviz/configs/specviz/plugins/viewers.py

+            self.state.x_display_unit = x_units if len(x_units) else None
+            self.state.y_display_unit = y_units if len(y_units) else None


I don't think len is needed.

Suggested change

self.state.x_display_unit = x_units if len(x_units) else None

self.state.y_display_unit = y_units if len(y_units) else None

self.state.x_display_unit = x_units if x_units else None

self.state.y_display_unit = y_units if y_units else None

woops, turns out it was necessary (CI fails everywhere otherwise)

Hmm, weird, but ok.

jdaviz/configs/specviz/plugins/viewers.py

pllim · 2023-04-13T18:45:16Z

jdaviz/core/helpers.py

                    flux_unit = self.app._get_display_unit('flux')
                    # TODO: any other attributes (meta, wcs, etc)?
                    # TODO: implement uncertainty.to upstream
+                    new_uncert = data.uncertainty.__class__(data.uncertainty.quantity.to(flux_unit)) if data.uncertainty is not None else None  # noqa


Wut... the uncertainty object cannot convert unit natively?

nope! I put a todo comment above, but probably should file an issue upstream. Probably in astropy.nddata?

pllim · 2023-04-13T18:46:01Z

jdaviz/core/marks.py

@@ -14,7 +14,7 @@

 __all__ = ['OffscreenLinesMarks', 'BaseSpectrumVerticalLine', 'SpectralLine',
           'SliceIndicatorMarks', 'ShadowMixin', 'ShadowLine', 'ShadowLabelFixedY',
-           'PluginMark', 'PluginLine', 'PluginScatter',
+           'PluginMark', 'LinesAutoUnit', 'PluginLine', 'PluginScatter',


This does not look related.

its needed for RTD to be able to pull in the API docs for the classes that inherit from it.

pllim · 2023-04-13T18:46:05Z

jdaviz/core/template_mixin.py

@@ -28,6 +28,7 @@


 __all__ = ['show_widget', 'TemplateMixin', 'PluginTemplateMixin',
+           'ViewerPropertiesMixin',


This does not look related.

(same as above - needed for RTD)

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

javerbukh

Things seem to work well in specviz but in cubeviz I'm not able to create a spectral subset after changing the units. I am able to create a spectral subset and then switch units, however.

kecnry · 2023-04-13T19:04:19Z

Things seem to work well in specviz but in cubeviz I'm not able to create a spectral subset after changing the units. I am able to create a spectral subset and then switch units, however.

This is known: #2125

javerbukh

@kecnry Got it, looks good otherwise!

pllim

Thanks!

pllim · 2023-04-13T19:20:48Z

Can't do automerge, maybe because this is not against main? 🤷

* fix error raised when spectrum uncertainty is None * fix RTD build failure because of missing mixin available in import * fix test setting display unit now that um is preferred to micron * add disabled unit conversion plugin to mosviz * avoid unit-conversion error in cubeviz tests * bump versions of glue/glue-jupyter * fix deg intialization in cubeviz slice plugin * add LinesAutoUnit to __all__ so RTD can find it * add use_display_units kwarg to get_spectral_regions * fix setting display units for unitless data * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* bump glue-core to necessary version for display units * cubeviz/slice plugin unit conversion support (we might want to generalize some of this logic more into the mark's _update_data when adding support for spectral lines as well) * display units: fix failing tests (#2132) * fix error raised when spectrum uncertainty is None * fix RTD build failure because of missing mixin available in import * fix test setting display unit now that um is preferred to micron * add disabled unit conversion plugin to mosviz * avoid unit-conversion error in cubeviz tests * bump versions of glue/glue-jupyter * fix deg intialization in cubeviz slice plugin * add LinesAutoUnit to __all__ so RTD can find it * add use_display_units kwarg to get_spectral_regions * fix setting display units for unitless data * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * remove unit conversion plugin from all bug specviz * for initial implementation - eventually we'll want to implement across all plugins/viewers * Working on unit conversion handling in subset plugin * Get subset updating working in non-default units * Add changelog * Update jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Added .ico and .svg files * app-wide display unit in specviz (#2048) * 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> * bump glue-core to necessary version for display units * change PR number in changelog to main PR * Display units line analysis (#2128) * emit event when display unit changed * update line analysis for unit changes * cubeviz/slice plugin unit conversion support (we might want to generalize some of this logic more into the mark's _update_data when adding support for spectral lines as well) * Display units support for markers plugin (#2130) * update markers plugin for display units * move unit-updating logic to mark itself * LineUncertainties to be unit-aware * fix support for markers in cubeviz * display units: fix failing tests (#2132) * fix error raised when spectrum uncertainty is None * fix RTD build failure because of missing mixin available in import * fix test setting display unit now that um is preferred to micron * add disabled unit conversion plugin to mosviz * avoid unit-conversion error in cubeviz tests * bump versions of glue/glue-jupyter * fix deg intialization in cubeviz slice plugin * add LinesAutoUnit to __all__ so RTD can find it * add use_display_units kwarg to get_spectral_regions * fix setting display units for unitless data * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Display units: line lists (#2148) * BaseSpectrumVerticalLine to inherit unit-support from PluginMark * convert units on internal rest value * code cleanup * remove unit conversion plugin from all bug specviz * for initial implementation - eventually we'll want to implement across all plugins/viewers * get_display_unit fallback for configs without unit conversion plugin * Update glue-core dependency * Update Subset Tools plugin to use display units (#2195) * bump glue-core to necessary version for display units * cubeviz/slice plugin unit conversion support (we might want to generalize some of this logic more into the mark's _update_data when adding support for spectral lines as well) * display units: fix failing tests (#2132) * fix error raised when spectrum uncertainty is None * fix RTD build failure because of missing mixin available in import * fix test setting display unit now that um is preferred to micron * add disabled unit conversion plugin to mosviz * avoid unit-conversion error in cubeviz tests * bump versions of glue/glue-jupyter * fix deg intialization in cubeviz slice plugin * add LinesAutoUnit to __all__ so RTD can find it * add use_display_units kwarg to get_spectral_regions * fix setting display units for unitless data * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * remove unit conversion plugin from all bug specviz * for initial implementation - eventually we'll want to implement across all plugins/viewers * Working on unit conversion handling in subset plugin * Get subset updating working in non-default units * Add changelog * Update jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * allow downstream apps to set what classes are considered native marks * Fix unit conversion for PluginMarks and y-limits (#2215) * Fix unit conversion for PluginMarks and y-limits * Only use equivalency if it's a spec viewer * Store wavelength unit on data update * Remove redundant line * Better check in marks * Replace subset_to_apply in docs where possible (#2223) * Replace subset_to_apply in docs where possible (#2223) * Avoid trying to convert units of empty marks arrays * Add open method to automatically detect config and load data * Update demo notebooks * Codestyle * Add autoconfig test coverage * Revert "Update demo notebooks" This reverts commit cc513b3. * Add mention of open to relevant example notebooks * Prepare to support open via cli * Remove the need to open file twice for autoconfig detection * Fix Helper vs App Docstring * TST: Change PIP_EXTRA_INDEX_URL because packages moved where they upload nightly builds. * Display units: model fitting (#2216) * model component compatibility based on display units * equation validity based on display units * allow re-estimating model parameters to update compat checks * test coverage * get_data(use_display_units) to pass spectral_density equivalency * handle whitespace in model equation * Add clarity for kwargs * plugin results: support overwriting when data loaded in multiple viewers (#2222) * reverts removal of call to set_plot_axes when loading data into viewer (#2235) * fixes a bug in the axes labels on cube viewers in cubeviz as well as setting the correct attributes in lcviz * Subsume failing identifier test into autoconfig test * Update subset plugin UI (#2234) * Update subset plugin UI * Apply suggestions from code review Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> * Changelog * Add docs page to explain how to create jdaviz-readable products (#2228) * Add page for data products * Update index * Fix title * Capital letters * Address first review comments * Edit format * Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> * Including reference in import_data * Update docs/create_products.rst * Update docs/create_products.rst --------- Co-authored-by: Camilla Pacifici <cpacifici@stsci.edu> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> * FEAT: Load annulus from file and allow IMPORT DATA to load reg files. Annulus support provided by glue-viz/glue#2403 and glue-viz/glue-astronomy#92 Add test for exception. Bump minimum requirements for glue-core and glue-astronomy. * disallow gaussian smooth output as input * BUG: Fix ellipse translation in app.get_subsets() * DOC: Use pydata-sphinx-theme (#2243) * 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 (#2237) * 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> * Fix missed conflict --------- Co-authored-by: Jennifer Kotler <jkotler@esme.stsci.edu> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> Co-authored-by: Jesse Averbukh <javerbukh@gmail.com> Co-authored-by: Ricky O'Steen <rosteen@stsci.edu> Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com> Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com> Co-authored-by: Camilla Pacifici <cpacifici@stsci.edu>

kecnry added the no-changelog-entry-needed changelog bot directive label Apr 5, 2023

kecnry added this to the 3.5 milestone Apr 5, 2023

kecnry changed the base branch from main to dev-display-units April 5, 2023 19:16

github-actions bot added mosviz specviz labels Apr 5, 2023

kecnry force-pushed the display-units-fix-ci branch from 33cbf2b to d3c60ba Compare April 5, 2023 19:20

This comment was marked as resolved.

Sign in to view

kecnry mentioned this pull request Apr 11, 2023

Display units (unit conversion refactor) #2127

Merged

19 tasks

kecnry added 5 commits April 11, 2023 10:10

fix error raised when spectrum uncertainty is None

c879296

fix RTD build failure because of missing mixin available in import

970836d

fix test setting display unit now that um is preferred to micron

c1e19b7

add disabled unit conversion plugin to mosviz

b4e5d3e

avoid unit-conversion error in cubeviz tests

f6058f2

kecnry force-pushed the display-units-fix-ci branch from d3c60ba to f6058f2 Compare April 11, 2023 14:11

kecnry added 4 commits April 13, 2023 09:17

bump versions of glue/glue-jupyter

83994c8

fix deg intialization in cubeviz slice plugin

3690143

add LinesAutoUnit to __all__ so RTD can find it

ee12c73

add use_display_units kwarg to get_spectral_regions

c4bb23d

kecnry commented Apr 13, 2023

View reviewed changes

kecnry force-pushed the display-units-fix-ci branch from 7c5394f to 901482b Compare April 13, 2023 17:42

fix setting display units for unitless data

a41c92f

kecnry force-pushed the display-units-fix-ci branch from 901482b to a41c92f Compare April 13, 2023 17:54

kecnry marked this pull request as ready for review April 13, 2023 18:35

kecnry requested review from duytnguyendtn, rosteen, javerbukh, pllim, bmorris3 and haticekaratay as code owners April 13, 2023 18:35

kecnry requested a review from cshanahan1 as a code owner April 13, 2023 18:35

pllim reviewed Apr 13, 2023

View reviewed changes

kecnry force-pushed the display-units-fix-ci branch from d914fd8 to 0849937 Compare April 13, 2023 18:54

Apply suggestions from code review

f030cce

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

kecnry force-pushed the display-units-fix-ci branch from 0849937 to f030cce Compare April 13, 2023 18:58

fix RTD link in API docstring

7bda40b

javerbukh reviewed Apr 13, 2023

View reviewed changes

javerbukh approved these changes Apr 13, 2023

View reviewed changes

kecnry added the Ready for final review label Apr 13, 2023

pllim approved these changes Apr 13, 2023

View reviewed changes

kecnry merged commit 819ed17 into spacetelescope:dev-display-units Apr 13, 2023

kecnry deleted the display-units-fix-ci branch April 13, 2023 19:23

kecnry mentioned this pull request Apr 13, 2023

Display units: line lists #2148

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

display units: fix failing tests #2132

display units: fix failing tests #2132

kecnry commented Apr 5, 2023 •

edited by pllim

Loading

This comment was marked as resolved.

kecnry Apr 13, 2023

pllim Apr 13, 2023

codecov bot commented Apr 13, 2023 •

edited

Loading

pllim left a comment

pllim Apr 13, 2023

pllim Apr 13, 2023

kecnry Apr 13, 2023

pllim Apr 13, 2023

pllim Apr 13, 2023

kecnry Apr 13, 2023

pllim Apr 13, 2023

kecnry Apr 13, 2023

pllim Apr 13, 2023

kecnry Apr 13, 2023

javerbukh left a comment

kecnry commented Apr 13, 2023

javerbukh left a comment

pllim left a comment

pllim commented Apr 13, 2023

		self.state.x_display_unit = x_units if len(x_units) else None
		self.state.y_display_unit = y_units if len(y_units) else None

		@@ -28,6 +28,7 @@


		__all__ = ['show_widget', 'TemplateMixin', 'PluginTemplateMixin',
		'ViewerPropertiesMixin',

display units: fix failing tests #2132

display units: fix failing tests #2132

Conversation

kecnry commented Apr 5, 2023 • edited by pllim Loading

Description

Change log entry

Checklist for package maintainer(s)

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 13, 2023 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Apr 13, 2023

javerbukh left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim commented Apr 13, 2023

kecnry commented Apr 5, 2023 •

edited by pllim

Loading

codecov bot commented Apr 13, 2023 •

edited

Loading