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

Redraw empty histogram layer #2300

Merged
merged 5 commits into from
Aug 4, 2022
Merged

Conversation

Carifio24
Copy link
Member

This PR addresses an issue that was discussed at a glue meeting a little while ago (don't remember the exact date). The issue is that if a subset group is modified to make it empty, the corresponding histogram layer artist is not redrawn. See the video below.

histogram-bug.mp4

The issue seems to be here, where _update_artists return if the sum of the histogram is empty (that is, the layer has no content). To fix this, I've removed the mpl_hist.sum() == 0 check. Allowing the possibility for empty layer opens the door for the largest_y_max and smallest_y_min values computed later in the method to be NaN or infinite (in particular, if all of the layers are empty). I couldn't think of a better default behavior than just punting on updating the bounds in this case, so I've added numpy.isfinite checks around both assignments to the relevant viewer state properties.

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #2300 (dd82bc8) into main (e1331ec) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #2300      +/-   ##
==========================================
- Coverage   88.14%   88.11%   -0.04%     
==========================================
  Files         247      247              
  Lines       23365    23369       +4     
==========================================
- Hits        20596    20591       -5     
- Misses       2769     2778       +9     
Impacted Files Coverage Δ
glue/viewers/histogram/state.py 92.15% <0.00%> (-2.48%) ⬇️
glue/viewers/histogram/layer_artist.py 93.04% <100.00%> (-0.87%) ⬇️
glue/conftest.py 63.09% <0.00%> (-4.77%) ⬇️

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 e1331ec...dd82bc8. Read the comment docs.

@astrofrog
Copy link
Member

@Carifio24 - this looks good! Could you add a test and changelog entry?

@Carifio24
Copy link
Member Author

@astrofrog I've added a changelog entry and a test (TestHistogramViewer::test_redraw_empty_subset) that checks whether all of the Rectangles in the layer artist have height zero after the subset state is set to an empty one.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

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