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

Disable resetting of y_min/y_max in layer artist #247

Merged
merged 9 commits into from
Sep 16, 2021

Conversation

astrofrog
Copy link
Member

I need to figure out if this code is still needed, but for now it seems it definitely isn't for specviz, so this branch can be used with specviz to avoid rescaling all the time.

@codecov
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #247 (42c0f7f) into main (7e096c7) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   88.63%   88.58%   -0.06%     
==========================================
  Files          82       82              
  Lines        4093     4136      +43     
==========================================
+ Hits         3628     3664      +36     
- Misses        465      472       +7     
Impacted Files Coverage Δ
glue_jupyter/bqplot/profile/viewer.py 82.60% <ø> (-0.73%) ⬇️
glue_jupyter/bqplot/histogram/layer_artist.py 91.66% <100.00%> (ø)
glue_jupyter/bqplot/image/layer_artist.py 88.46% <100.00%> (ø)
glue_jupyter/bqplot/profile/layer_artist.py 92.30% <100.00%> (-0.90%) ⬇️
glue_jupyter/utils.py 62.38% <0.00%> (-3.04%) ⬇️
glue_jupyter/conftest.py 100.00% <0.00%> (ø)
glue_jupyter/common/slice_helpers.py 0.00% <0.00%> (ø)
glue_jupyter/tests/test_state_traitlets_helpers.py 100.00% <0.00%> (ø)
glue_jupyter/bqplot/scatter/layer_artist.py 99.42% <0.00%> (+<0.01%) ⬆️
glue_jupyter/tests/main_test.py 98.44% <0.00%> (+0.01%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e096c7...42c0f7f. Read the comment docs.

@pllim
Copy link
Contributor

pllim commented Aug 9, 2021

If it is somehow needed elsewhere, Jdaviz still need an option to turn it off.

@pllim
Copy link
Contributor

pllim commented Aug 9, 2021

I can confirm that this also fixes the issue downstream at spacetelescope/jdaviz#760 , in addition to spacetelescope/jdaviz#335

@pllim
Copy link
Contributor

pllim commented Aug 9, 2021

@camipacifici reported that this might have broken the "Home" button, as pressing Home now does not reset Y-axis zoom in Specviz, though it works fine for Imviz.

@astrofrog
Copy link
Member Author

I've updated this to a point where I think I'd be happy to merge. I've also fixed the Home button.

At this point it would be helpful if someone could test this out with jdaviz/specviz though?

cc @camipacifici @pllim

@rosteen
Copy link
Contributor

rosteen commented Sep 15, 2021

I just confirmed that selecting a subset in the profile viewer no longer resets the y-axis zoom, and that the home button now properly resets both axes again in the profile viewer. Looks good!

@pllim
Copy link
Contributor

pllim commented Sep 15, 2021

I'll have to double check that the markers no longer reset Imviz zoom/pan, unless someone gets to it first.

@javerbukh
Copy link
Contributor

When using mosviz, selecting a new row does not change the Y-axis of the spectrum viewer at all. The scale of the first row persists for all the rest, even if the range is much different.

@pllim
Copy link
Contributor

pllim commented Sep 15, 2021

My comment at #247 (comment) is no longer true. This does not prevent pan/zoom reset in Imviz no more when markers are added. Not sure what changed.

But the Imviz issue is not important to fix right now.

@astrofrog
Copy link
Member Author

This PR never had any impact on the image viewer so I wouldn't expect it to fix your issue @pllim - but if you open a separate issue I can look into it

@pllim
Copy link
Contributor

pllim commented Sep 15, 2021

This PR never had any impact on the image viewer

It did at one point... 😆

But don't worry about Imviz for now.

@pllim
Copy link
Contributor

pllim commented Sep 15, 2021

But since you asked, I did open an issue at spacetelescope/jdaviz#760

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

This actually breaks jdaviz... See spacetelescope/jdaviz#874 . Maybe we need this is a very localized section of Jdaviz only...

@astrofrog
Copy link
Member Author

I am working on this in glue-viz/glue#2233

@astrofrog astrofrog merged commit efd9434 into glue-viz:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants