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

Fixes to profile viewer #2233

Merged
merged 9 commits into from
Sep 16, 2021
Merged

Conversation

astrofrog
Copy link
Member

This fixes three issues with the profile viewer:

  • The home button now correctly resets the y limits
  • The visibility checkbox for profile viewer layers now works correctly
  • The limits of the profile viewer are now no longer reset every time e.g. a subset is updated or a dataset is added - instead they are reset only when requested by the user or if the reference data changes (as for the image viewer)

@astrofrog astrofrog added the bug label Sep 16, 2021
@astrofrog astrofrog added this to the v1.2.2 milestone Sep 16, 2021
@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #2233 (fd39853) into main (3ea1ec6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2233      +/-   ##
==========================================
+ Coverage   88.02%   88.04%   +0.01%     
==========================================
  Files         247      247              
  Lines       23164    23172       +8     
==========================================
+ Hits        20391    20401      +10     
+ Misses       2773     2771       -2     
Impacted Files Coverage Δ
glue/plugins/dendro_viewer/layer_artist.py 89.47% <100.00%> (ø)
glue/viewers/histogram/layer_artist.py 93.91% <100.00%> (ø)
glue/viewers/image/layer_artist.py 88.93% <100.00%> (ø)
glue/viewers/profile/layer_artist.py 94.44% <100.00%> (-0.66%) ⬇️
glue/viewers/profile/state.py 98.95% <100.00%> (+0.12%) ⬆️
glue/viewers/scatter/layer_artist.py 97.21% <100.00%> (ø)
glue/plugins/dendro_viewer/data_factory.py 97.72% <0.00%> (-2.28%) ⬇️
glue/core/qt/layer_artist_model.py 84.78% <0.00%> (+1.08%) ⬆️

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 3ea1ec6...fd39853. Read the comment docs.

This should now be consistent with the image viewer: limits are only reset if the reference data changes, not every time a subset or dataset change or are added. This also fixes the home button to correctly reset the y limits.
@pllim
Copy link
Contributor

pllim commented Sep 16, 2021

This fixes #2223 , right?

# Make sure the limits don't change if a subset is created or another
# dataset added - they should only change if the reference data is changed
self.viewer.add_data(self.data)
self.viewer.state.x_min = 0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if allclose check is good enough. You know floating point and all that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is fine as it should literally return the exact same values, there is no math involved :)

# A callback event for reference_data is triggered if the choices change
# but the actual selection doesn't - so we avoid resetting the WCS in
# this case.
if before is after:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how multiverse is created? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see!

@astrofrog astrofrog merged commit 39f361d 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants