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

Fix bug that caused incompatible subsets to be rendered in some contexts rather than be hidden #2425

Merged

Conversation

jfoster17
Copy link
Member

@jfoster17 jfoster17 commented Jul 14, 2023

Pull Request Template

Description

This is my attempt that fixes glue-viz/glue-jupyter#367 and is at least a reasonable solution to #2405. I'm still not 100% sure that this is correct approach, but it seems to work and passes all current tests.

  • Understand the workflow that requires us to compute the mask here for IncompatibleAttributes and make sure this solution still works for that.

Fixes #2405
Fixes glue-viz/glue-jupyter#367

@astrofrog
Copy link
Member

@jfoster17 regarding your question in #2405 about why we create a large NaN array, I have to admit I'm not sure anymore why we had to do this - but it might very much be the case that we don't need to anymore so if everything works fine with the approach in this PR (both in Qt and Jupyter glue) then I'm ok with the solution!

@jfoster17 jfoster17 marked this pull request as ready for review September 15, 2023 15:35
@jfoster17
Copy link
Member Author

After poking at this a bit more, we don't actually have to return anything here for incompatible layers, which is cleaner.

It's not easy to write a test for this in glue-core, since this is sort of inherently a front-end thing, but one that hits both glue-core and glue-qt. @astrofrog : Thoughts on how to proceed with testing this?

(The current test failures are unrelated and due to the new version of Matplotlib 3.8.0)

@jfoster17 jfoster17 added the bug label Sep 15, 2023
@astrofrog
Copy link
Member

This works, thanks! I tried setting up a regression test in glue-jupyter: glue-viz/glue-jupyter#404 - however for some reason while the test seems to reproduce the failure locally, it does not when run on CircleCI. In any case we should merge and release this.

@astrofrog astrofrog changed the title Set incompatible image subset from bounds Fix bug that caused incompatible subsets to be rendered in some contexts rather than be hidden Oct 23, 2023
@astrofrog astrofrog merged commit df7599f into glue-viz:main Oct 23, 2023
20 of 21 checks passed
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.

Incompatible subsets turn the image viewer green Incompatible Subsets use too much memory
2 participants