glue-viz / glue

Linked Data Visualizations Across Multiple Files

http://glueviz.org

Other

742 stars 153 forks source link

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

Closed jfoster17 closed 1 year ago

jfoster17 commented 1 year ago

Pull Request Template

Description

This is my attempt that fixes https://github.com/glue-viz/glue-jupyter/issues/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.

[x] 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 https://github.com/glue-viz/glue-jupyter/issues/367

astrofrog commented 1 year ago

@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 commented 1 year ago

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)

astrofrog commented 1 year ago

This works, thanks! I tried setting up a regression test in glue-jupyter: https://github.com/glue-viz/glue-jupyter/pull/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.