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.

Sign up for GitHub

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

Jump to bottom

reverts removal of call to set_plot_axes when loading data into viewer #2235

Merged
merged 1 commit into from
Jun 2, 2023
Merged

reverts removal of call to set_plot_axes when loading data into viewer #2235

merged 1 commit into from
Jun 2, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Jun 2, 2023 •
edited
Loading

Description

This pull request fixes a bug in the dev-display-units (#2127) branch with the axes labels on cube viewers in cubeviz. An internal call to set_plot_axes when new data is added to a viewer was inadvertantly removed in https://github.com/spacetelescope/jdaviz/pull/2048/files#diff-33416aee3e2d4c0d86f96811f082119130c8598ef01ce1c9a7e852ecc39779b3L1595 .

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
reverts removal of call to set_plot_axes when loading data into viewer
f1402b3 
* fixes a bug in the axes labels on cube viewers in cubeviz as well as setting the correct attributes in lcviz
@kecnry kecnry added the no-changelog-entry-needed changelog bot directive label Jun 2, 2023
@kecnry kecnry added this to the 3.6 milestone Jun 2, 2023
@kecnry kecnry mentioned this pull request Jun 2, 2023
Merged
19 tasks
@kecnry kecnry added the trivial Only needs one approval instead of two label Jun 2, 2023
@kecnry kecnry marked this pull request as ready for review June 2, 2023 16:17
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

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

Additional details and impacted files 
@@                 Coverage Diff                  @@
##             dev-display-units    #2235   +/-   ##
====================================================
  Coverage                     ?   91.64%           
====================================================
  Files                        ?      148           
  Lines                        ?    16660           
  Branches                     ?        0           
====================================================
  Hits                         ?    15268           
  Misses                       ?     1392           
  Partials                     ?        0

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

@kecnry kecnry merged commit a9151a4 into spacetelescope:dev-display-units Jun 2, 2023
@kecnry kecnry deleted the fix-set-plot-axes branch June 2, 2023 16:35
rosteen added a commit that referenced this pull request Jun 13, 2023
@kecnry @pllim
@javerbukh @rosteen @duytnguyendtn @camipacifici
Refactor Unit Conversion plugin in Specviz (#2127)
19f89e4 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duytnguyendtn duytnguyendtn Awaiting requested review from duytnguyendtn

@rosteen rosteen Awaiting requested review from rosteen

@javerbukh javerbukh Awaiting requested review from javerbukh

@pllim pllim Awaiting requested review from pllim

@bmorris3 bmorris3 Awaiting requested review from bmorris3

@haticekaratay haticekaratay Awaiting requested review from haticekaratay

@cshanahan1 cshanahan1 Awaiting requested review from cshanahan1

Assignees
No one assigned
Labels
no-changelog-entry-needed changelog bot directive trivial Only needs one approval instead of two
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
1 participant
@kecnry