future-proof slicing logic for glue-jupyter as_steps support #1599

kecnry · 2022-08-26T13:26:28Z

Description

This pull request future-proofs the slicing wavelength to slice logic for when a future release of glue-jupyter includes glue-viz/glue-jupyter#309. That PR, when layer.as_steps=True, doubles the length of the bqplot Lines object, which has been used since #1550 to quickly access the wavelength array to map wavelength to slice.

I suspect this will fail coverage, as we have no way within CI to force entering that if-statement. Covering the if-statement within CI should probably be added to #1595 (I added a TODO entry there to make sure that is done once that is rebased on top of this).

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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

rosteen · 2022-08-26T13:28:33Z

jdaviz/configs/cubeviz/helper.py

-        x_all = self.app.get_viewer('spectrum-viewer').native_marks[0].x
+        sv = self.app.get_viewer('spectrum-viewer')
+        x_all = sv.native_marks[0].x
+        if sv.state.layers[0].as_steps and GLUEJUPYTER_HAS_AS_STEPS:


Do these two clauses need to be in the opposite order to avoid an error if as_steps isn't present?

as_steps is present in the layer state as of glue 1.3.0 (and we require 1.5.0). But before glue-jupyter 309 is released, it does nothing within bqplot (and so the length does NOT double if someone manually changes the state of as_steps to True).

Got it. I'll test shortly.

kecnry · 2022-08-26T13:28:36Z

jdaviz/configs/cubeviz/helper.py

 from jdaviz.core.helpers import ImageConfigHelper
 from jdaviz.configs.default.plugins.line_lists.line_list_mixin import LineListMixin
 from jdaviz.configs.specviz import Specviz
 from jdaviz.core.events import (AddDataMessage,
                                SliceSelectSliceMessage)

+# NOTE: this and the if-statement that uses it can be removed if/once
+# the version of glue-jupyter with as_steps support is pinned as min-version
+GLUEJUPYTER_HAS_AS_STEPS = Version(gj_version) >= Version('1.3.0')


to test this, install glue-viz/glue-jupyter#309 locally and change GLUEJUPYTER_HAS_AS_STEPS to True (since the version check won't actually resolve to True until 309 is merged and released).

codecov · 2022-08-26T13:38:13Z

Codecov Report

Merging #1599 (36ced8b) into main (d70425c) will decrease coverage by 0.00%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #1599      +/-   ##
==========================================
- Coverage   86.12%   86.12%   -0.01%     
==========================================
  Files          94       94              
  Lines        9321     9326       +5     
==========================================
+ Hits         8028     8032       +4     
- Misses       1293     1294       +1

Impacted Files	Coverage Δ
jdaviz/configs/cubeviz/helper.py	`94.64% <83.33%> (-1.44%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rosteen

I confirmed that setting GLUEJUPYTER_HAS_AS_STEPS to True fixes the slicing behavior with the glue-jupyter PR that implements histogram-style plotting. Approved.

jdaviz/configs/cubeviz/helper.py

pllim · 2022-08-26T15:18:41Z

CHANGES.rst

@@ -39,6 +39,8 @@ Bug Fixes
 Cubeviz
 ^^^^^^^

+- Future proof slicing logic for as_steps implementation in upcoming glue-jupyter release. [#1599]


I am not sure if this needs a change log. User is not impacted yet, so this might just confuse them.

I wasn't sure where to put this, but my thinking was that jdaviz 2.9 (the latest release) will break for anyone who updates glue-jupyter to ~~1.3~~ 0.13 (once that is released)... so it will become a bugfix to 2.9 once that happens. If someone stays on 2.9, updates glue-jupyter, and sees the break, then maybe this entry will tell them that an update to 2.10 will fix their problem? But I also see your point, that at the time 2.10 is released, the bug doesn't actually exist yet.

Maybe more visible as a known issue entry then? "If you have this version but upgraded that package, this thing will break" under the Installation section.

except we can't go back and add that to the 2.9 docs afaik, which is where it would be really useful. In the future, especially once we pin glue-jupyter, that entry in the stable docs will be irrelevant.

I think our RTD defaults to latest, not stable.

But if you insist on the change log, then that is fine too. Mine was merely a suggestion.

ojustino · 2022-08-26T16:16:24Z

I attempted to review but am wondering if I'm missing something about glue-jupyter. I only see that it's at v0.12.1 on GitHub and PyPI, but the check is for v1.3. Is there somewhere else I should be looking?

kecnry · 2022-08-26T16:21:22Z

@ojustino - you'll need to install my profile-viewer-as-steps branch that is glue-viz/glue-jupyter#309 that will likely make it in glue-jupyter 0.13.0 (and good catch! I accidentally had 1.3.0 instead of 0.13.0... must've gotten confused with glue being at 1.3.0... fixing that now!!! 😬 )

ojustino

I tested two ways:

I pulled this branch and used pip install -e . to install its version of jdaviz in a new conda environment. That gave me glue-jupyter v0.12.0, which should trigger the False case of the new if statement.
I used pip install git+https://github.com/kecnry/glue-jupyter.git@profile-viewer-as-steps to install the indicated version of glue-jupyter, which appears for me as v0.10.2.dev.... (and so on). Since the glue-jupyterversion number decreased, I changed the minversion check to v0.10 in order to trigger the True case of the new if statement.

I didn't have a problem with slicing in Cubeviz either way, so I am OK approving since the code looks fine and it would be useful for others to get the release out soon.

kecnry · 2022-08-26T17:15:50Z

glue-viz/glue-jupyter#309 is now merged into main, and is causing CI to fail for the dev case. That's just because its not triggering the minversion check and so is essentially just exposing the bug if this isn't in place. Once that gets tagged as 0.13, local tests show that this should do the trick 🤞

pllim · 2022-08-26T17:25:04Z

Thanks for the quick fix!

and revert version check from spacetelescope#1599

to cover if-statement introduced in spacetelescope#1599

and revert version check from spacetelescope#1599

to cover if-statement introduced in spacetelescope#1599

* plot options switch for as_steps * for data layers, defaults to False, for subset layers: default to as_steps option on parent data layer (when subset layer is created) * internal uncertainty styling to obey "as steps" * bump glue-jupyter to 0.13 * add slice tests to cover if-statement introduced in #1599

kecnry added this to the 2.10 milestone Aug 26, 2022

github-actions bot added the cubeviz label Aug 26, 2022

kecnry force-pushed the slice-as-steps branch from 459fd78 to 8efd4ef Compare August 26, 2022 13:27

rosteen reviewed Aug 26, 2022

View reviewed changes

kecnry commented Aug 26, 2022

View reviewed changes

kecnry force-pushed the slice-as-steps branch from 8efd4ef to 1228872 Compare August 26, 2022 13:29

kecnry mentioned this pull request Aug 26, 2022

profile viewer as_steps support #1595

Merged

11 tasks

kecnry marked this pull request as ready for review August 26, 2022 13:36

kecnry requested review from duytnguyendtn, javerbukh, ojustino and pllim as code owners August 26, 2022 13:36

rosteen approved these changes Aug 26, 2022

View reviewed changes

kecnry added Ready for final review 🔥 Critical labels Aug 26, 2022

pllim reviewed Aug 26, 2022

View reviewed changes

jdaviz/configs/cubeviz/helper.py Outdated Show resolved Hide resolved

pllim reviewed Aug 26, 2022

View reviewed changes

kecnry force-pushed the slice-as-steps branch from 1228872 to 373f518 Compare August 26, 2022 16:11

kecnry force-pushed the slice-as-steps branch from 373f518 to 1de9242 Compare August 26, 2022 16:21

future-proof slicing logic for glue-jupyter as_steps support

36ced8b

kecnry force-pushed the slice-as-steps branch from 1de9242 to 36ced8b Compare August 26, 2022 16:45

ojustino approved these changes Aug 26, 2022

View reviewed changes

kecnry merged commit 6f7cb5a into spacetelescope:main Aug 26, 2022

kecnry deleted the slice-as-steps branch August 26, 2022 17:17

kecnry added a commit to kecnry/jdaviz that referenced this pull request Aug 26, 2022

bump glue-jupyter to 0.13

786c514

and revert version check from spacetelescope#1599

kecnry added a commit to kecnry/jdaviz that referenced this pull request Aug 26, 2022

add slice tests

463f0da

to cover if-statement introduced in spacetelescope#1599

kecnry added a commit to kecnry/jdaviz that referenced this pull request Aug 29, 2022

bump glue-jupyter to 0.13

a266277

and revert version check from spacetelescope#1599

kecnry added a commit to kecnry/jdaviz that referenced this pull request Aug 29, 2022

add slice tests

0e756ca

to cover if-statement introduced in spacetelescope#1599

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

future-proof slicing logic for glue-jupyter as_steps support #1599

future-proof slicing logic for glue-jupyter as_steps support #1599

kecnry commented Aug 26, 2022 •

edited

Loading

rosteen Aug 26, 2022

kecnry Aug 26, 2022

rosteen Aug 26, 2022

kecnry Aug 26, 2022

codecov bot commented Aug 26, 2022 •

edited

Loading

rosteen left a comment

pllim Aug 26, 2022

kecnry Aug 26, 2022 •

edited

Loading

pllim Aug 26, 2022

kecnry Aug 26, 2022

pllim Aug 26, 2022

ojustino commented Aug 26, 2022

kecnry commented Aug 26, 2022 •

edited

Loading

ojustino left a comment

kecnry commented Aug 26, 2022 •

edited

Loading

pllim commented Aug 26, 2022

future-proof slicing logic for glue-jupyter as_steps support #1599

future-proof slicing logic for glue-jupyter as_steps support #1599

Conversation

kecnry commented Aug 26, 2022 • edited Loading

Description

Checklist for package maintainer(s)

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

codecov bot commented Aug 26, 2022 • edited Loading

Codecov Report

rosteen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry Aug 26, 2022 • edited Loading

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

ojustino commented Aug 26, 2022

kecnry commented Aug 26, 2022 • edited Loading

ojustino left a comment

Choose a reason for hiding this comment

kecnry commented Aug 26, 2022 • edited Loading

pllim commented Aug 26, 2022

kecnry commented Aug 26, 2022 •

edited

Loading

codecov bot commented Aug 26, 2022 •

edited

Loading

kecnry Aug 26, 2022 •

edited

Loading

kecnry commented Aug 26, 2022 •

edited

Loading

kecnry commented Aug 26, 2022 •

edited

Loading