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: use last focused/resized view to determine shape of BqplotImageView #475

Conversation

iisakkirotko
Copy link
Contributor

@iisakkirotko iisakkirotko commented Dec 11, 2024

Pull Request Template

Description

Instead of always using the lowest cid view to determine the aspect ratio / shape of a Bqplot image viewer, use the last focused / resized one. focused_at describes the time that the window of that view was last focused, while resized_at describes when each viewer was last resized, so this way distortion from resizing based on a view in a popout window will be fixed once the original window is brought into focus.

Needs glue-viz/bqplot-image-gl#112 to work. Let me know if you need this to use the old behaviour as a fallback with previous versions of bqplot-image-gl.

TODO:

@maartenbreddels
Copy link
Collaborator

Let me know if you need this to use the old behaviour as a fallback with previous versions of bqplot-image-gl.

I think it's fine like this, if we also bump

bqplot-image-gl>=1.5.0

Otherwise some people might get the old behaviour.

@maartenbreddels
Copy link
Collaborator

We can use version 1.6.0 now (https://pypi.org/project/bqplot-image-gl/1.6.0/)

@iisakkirotko iisakkirotko force-pushed the fix-use-last-focused-resized-view-for-shape-bqplotimageview branch from 9625a7c to 1630099 Compare December 27, 2024 10:36
@iisakkirotko iisakkirotko marked this pull request as ready for review January 21, 2025 15:39
@dhomeier
Copy link
Contributor

Could you rebase to try and get the tests running?

@iisakkirotko iisakkirotko force-pushed the fix-use-last-focused-resized-view-for-shape-bqplotimageview branch from 1630099 to 7562d4a Compare January 29, 2025 10:17
@iisakkirotko
Copy link
Contributor Author

@dhomeier done!

@dhomeier
Copy link
Contributor

dhomeier commented Jan 29, 2025

@maartenbreddels: visual failure is related to astropy/astropy#17683 / widgetti/solara#990, right?

@maartenbreddels
Copy link
Collaborator

Indeed

@dhomeier
Copy link
Contributor

Hmm, updating to solara 1.44 seems to have broken more than it fixed – any ideas what those pytest-playwright failures are?

@dhomeier
Copy link
Contributor

dhomeier commented Jan 30, 2025

The last run prior to upgrading solara already was at pytest-playwright==0.6.2 so thought it was already pinned somehow. But I think that already raised on an earlier point in collection, so perhaps does not tell us much.

@maartenbreddels
Copy link
Collaborator

maartenbreddels commented Jan 30, 2025

Downgrading pytest-playwright helped, but now I hit widgetti/solara#606 - which I can now reproduce (so working on that now).

@dhomeier
Copy link
Contributor

dhomeier commented Feb 4, 2025

Log looks like two comparisons failed, but I only see one in
test_contour_units_chromiumtest_contour_units_chromium
Changed background colour probably would not be critical, but any idea where it comes from?

setup.cfg Outdated
pytest-mpl
solara[pytest]<1.29
pytest-ipywidgets[all]
solara-ui @ https://github.com/widgetti/solara/archive/refs/heads/fix_matplotlib_backend_keyerror.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Something therein is pinning jupyter-client 7.x, jupyterlab 3.x and notebook 6.x now, among others.
But for visualtest only we probably can/have to live with that for now.

Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

Approving as is, but if you want to pull the CI fixes out to further investigate the frame colour issue @maartenbreddels, fine with me, too.

iisakkirotko and others added 6 commits February 18, 2025 13:18
As mentioned in widgetti/solara#913
pytest-ipywidgets is not compatible with newer versions of
pytest-playwright.
We did not notice this in solara, because we pinned playwright, which
seems to limit the versions of pytest-playwright to a compatible one.
If we apply the same pinning as solara, we should be able to avoid
this issue.
@maartenbreddels maartenbreddels force-pushed the fix-use-last-focused-resized-view-for-shape-bqplotimageview branch from 42b4706 to 2e5310b Compare February 18, 2025 12:19
@maartenbreddels
Copy link
Collaborator

I've digged into why this is happening, and I think this is not a related change, and we should be able to merge this.

It seems that the output we see now is correct, since I see that bqplot-image-gl gets fed the border with values 0.2 0.2 0.2 1. which is an opaque dark grey. However, I don't understand why the current reference image is white. This might be a change in glue (maybe @astrofrog can confirm).

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.02%. Comparing base (52a3c29) to head (2e5310b).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
glue_jupyter/bqplot/image/viewer.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #475   +/-   ##
=======================================
  Coverage   86.01%   86.02%           
=======================================
  Files          91       91           
  Lines        5271     5272    +1     
=======================================
+ Hits         4534     4535    +1     
  Misses        737      737           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maartenbreddels
Copy link
Collaborator

@dhomeier if you agree, please rebase and merge, not squash.

@dhomeier
Copy link
Contributor

Can't see the plots now, since the Image Comparison page redirects to a nonexistent server, but sounds OK, thanks!

@dhomeier dhomeier merged commit 453f2c4 into glue-viz:main Feb 20, 2025
25 of 26 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.

3 participants