Open Modjular opened 6 months ago
This looks cool! Thanks for contributing! Our examples need a title, a tag, and a (brief) description--you can see the documentation here: https://napari.org/dev/developers/contributing.html#adding-examples-to-the-gallery This is causing the docs build to fail right now. If you don't have bandwidth for that, we can take care of it though! ❤️
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 92.43%. Comparing base (
9836ed1
) to head (b34fccc
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
So we have a small problem. The test fails are spurious and I've restarted them, but the example doesn't run: https://output.circle-artifacts.com/output/job/f13b5a24-1f6c-4682-a82c-ce5bf68c66f1/artifacts/0/docs/docs/_build/gallery/cursor_sync_custom_overlay.html#sphx-glr-gallery-cursor-sync-custom-overlay-py
But the example is passing test: https://github.com/napari/napari/actions/runs/7065938491/job/19238534492?pr=6510#step:11:167
So I suspect the difference is between the napari used for the tests and for docs. ~I think docs are using a pip install, so should be 0.4.18. But I think that overlays 2.0 should have already been released?~ Edit: nope, looks like the repo is cloned and installed from that in the docs workflows, so the plot thickens.
@brisvag do you have any ideas?
Edit2: I did some testing: the example here works fine in napari 0.4.18 and 0.4.19rc, but I get the same error as in the docs when using main
:
Traceback (most recent call last):
File "/Users/piotrsobolewski/Dev/napari/examples/cursor_sync_custom_overlay.py", line 106, in <module>
sync_cursor(parent, child)
File "/Users/piotrsobolewski/Dev/napari/examples/cursor_sync_custom_overlay.py", line 72, in sync_cursor
child.window._qt_viewer._add_overlay(cursor_overlay) # trigger overlay generation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'QtViewer' object has no attribute '_add_overlay'
So it seems like our examples test isn't catching this.
Edit3: so this PR is the cause of the difference: https://github.com/napari/napari/pull/5432 https://github.com/melonora/napari/blob/7a5c027e4f205a5c4118febc0b28e8172db2ceef/napari/_vispy/canvas.py#L583-L592 it has 0.5.0 milestone.
So I'm not sure the best way forward:
main
and milestone for 0.5.0? But then this example will stay in the latest
(dev) docs gallery and won't work for people coming from the image.sc thread.CC: @jni
Edit4: the coupe de grace: the example I went to reference when this first popped up also doesn't run on main with the same error: https://github.com/napari/napari/blob/main/examples/dev/overlays.py
Traceback (most recent call last):
File "/Users/piotrsobolewski/Dev/napari/examples/dev/overlays.py", line 88, in <module>
viewer.window._qt_viewer._add_overlay(viewer._overlays['dot'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'QtViewer' object has no attribute '_add_overlay'
So I think examples tests dont use Qt, so maybe that's why this example passes with main
?
https://github.com/napari/napari/blob/bb25bda71c1cbb2af4174cbb8963c645254173a3/.github/workflows/test_pull_requests.yml#L167-L176
Edit: I merged https://github.com/napari/docs/pull/256 So building docs should fail if an example fails.
Hi @Modjular, thanks for opening this PR and welcome to napari! Cool example. Given your example I will shamelessly try to recruit you to help contribute to multichannel / canvas view in napari. Would you be interested in that? Below a small sneak peak:
https://github.com/napari/napari/assets/22346363/4cb00bad-0e88-47db-bf9a-0cedeb864129
@melanora
Hi @Modjular, thanks for opening this PR and welcome to napari! Cool example. Given your example I will shamelessly try to recruit you to help contribute to multichannel / canvas view in napari. Would you be interested in that? Below a small sneak peak:
napari_2d_example.mp4
Cool! What are the goals of this feature? Is there a branch or README I could look at?
Cool! What are the goals of this feature? Is there a branch or README I could look at?
https://github.com/napari/docs/pull/249. The dicussion in the PR and the text of the NAP itself are both relevant :)
0.4.19 is out, so to get this in—which I definitely want—I think from: https://github.com/napari/napari/pull/6510#issuecomment-1836942282
we need to go with:
fix this PR for main and milestone for 0.5.0
Can post the 0.4.19 version in the image.sc thread, but we need a working version in main.
@Modjular I'm happy to do that to get this over the line if you'd like.
OK, so the example is passing tests because the way examples are tested with runpy.run_path
it won't execute code under if __name__ == "__main__":
so it just passes. We can see from the docs building that in fact the example is broken, due to the overlay changes as noted above.
I'm going to push fixes to both of these issues.
Ok, looks like everything is good now: https://output.circle-artifacts.com/output/job/ec34d49e-f402-44b2-b11b-2050a7c3a49b/artifacts/0/docs/docs/_build/html/gallery/cursor_sync_custom_overlay.html#sphx-glr-gallery-cursor-sync-custom-overlay-py
@jni ready for review!
Here's how it looks now, with the formatting improvement: https://output.circle-artifacts.com/output/job/2f3c51ae-9252-4322-a4d8-1baeb06da033/artifacts/0/docs/docs/_build/html/gallery/cursor_sync_custom_overlay.html#sphx-glr-gallery-cursor-sync-custom-overlay-py
@jni @brisvag If you have wording suggestions, please use the suggest feature!
@psobolewskiPhD my suggestion would be to do it the way I described above, cause it's more in line with how we use overlays inside napari core. (https://github.com/napari/napari/pull/6510#discussion_r1601151765)
I'm sorry @brisvag @jni I don't quite follow the conversation. I was just trying to fix this to get it over the line. Y'all are going to have to clue me in a bit more to rework it. ❤️
Basically, I think it would be best to move the whole syncing logic to be inside the VispyCursorOverlay
definition, like we do with all the other overlays. This way you don't need any extra functions to manually link and carry around, but instead we can use the normal mechanism of just doing viewer._overlays['something'] = something
(we need to update a line in napari to get automated linking post-init but that's easy and was supposed to happen anyways).
Note that the main difference between what you did and what I'm suggesting is that there's no explicit sync between two cursors overlays; rather, it's the same overlay model that's given to two different viewers, and you get the syncing for free, if you did the linking correctly!
Take a look at https://github.com/napari/napari/blob/main/napari/_vispy/overlays/brush_circle.py, it looks very much like what you want here.
The be
OK, I'll try to take another look. Just to be clear I didn't write this example, it's a user contribution via image.sc forums (see the OP) that withered due to the differences in overlays between release and main. I want(ed) to get it over the line for the first-time contributor, even though I think they've given up hope.
Yeah, I get it... But since this is an example meant for people to base their work on, I think it's best we write it in a way that aligns with how we want to use this feature ourself and in the future :)
References and relevant issues
@jni recommended I upload my solution from what was originally a question on the forums: https://forum.image.sc/t/creating-a-separate-cursor-in-the-view/88608
Description
I was originally trying to emulate ImageJ's "sync" feature. FOr simplicity's sake, I've only included the code for the cursor.
On running the example, you should a "parent" and "child" viewer. The child viewer should contain a red dot that mirrors the cursor's position, relative to the parent's window:
https://github.com/napari/napari/assets/8238329/80bb11b4-f4e1-4bb3-a02b-311d486bc9aa