-
Notifications
You must be signed in to change notification settings - Fork 9
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
feature/add-scene-selection-widget #25
feature/add-scene-selection-widget #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! @psobolewskiPhD this is great! Thanks for getting this work started!
I left some comments about the code and will also answer your questions here:
-
Yes, sorry about the
make build
requiringaicspylibczi
... We really need to get that built on an M1 mac somehow... @heeler @toloudis any ideas? -
I am generally okay with it for now, would love to see better support for these operations in general though. I will ask around for more opinions. Gimme a day or so on this PR.
-
I didn't know that
readlif
was going to provide access to scene names. Sounds like I may (or you or someone else) may need to make an easy change up-stream to baseaicsimageio
when that happens. We currently just generate these scene names. -
I think if you write a test that simply checks for the existance of the widget I would be happy with that for now personally. Or even better, checking the contents of the widget. I.e. Given a file, check the widget exists and check that it has 3 text rows with strings "1 -- foo", "2 -- bar", "3 -- baz".
onto some other comments.
- While
make build
may fail, you should be able to run the tests locally withpytest napari_aicsimageio/tests/
- I am seeing some lint and formatting errors you can check those locally with
tox -e lint
- It seems that the tests now require
PySide
.... Don't really know how that happened but could you addPySide
to the dependencies? Or I think updating ournapari
dependency tonapari[pyside]
should do the trick.
I will assume that we can get this into the library soonish. I will also be spending some time on resolving your other issues later this week (I plan on Friday). So hopefully if all goes well here and with other issues we can see a new release of this functionality by end of week 👍
Congratulations to this PR and thanks for the credit! As a side note, no need to include any license/acknowledgement from my side (declaring the _get_reader function a non-substantial portion of napari-imc here 😇) |
Another side note: this PR seems to be yet another use case of napari/napari#2229, which would resolve the kludge - happy to chat about this anytime |
It does build for me locally, not sure why not with pip. I can provide it.
I hacked together a way to get full scene names in
I think this should be possible. But I also thought the plugin/widget bit would be easy 🤣 Regarding the other comments: |
OK, Noob question: how do I go about making edits now? Do I checkout this PR? |
This is just because we don't build and release a version for M1 macs on PyPI because we don't have access to M1 machines for building I believe.
Good to know it will just slot in to
First off, really, thank you! Super happy to both see this in action and to see contribution. Second, a way to test this may be to return the widget from the function, obviously you don't need to do anything with it anywhere else, but if you return the widget, you can write a test that calls the function then checks the contents. OR, the "better" practice here would be to split up the "list of scene id text generation and tuple of scene id text and data" from the qt widget code. In fact I might propose factoring it out that way. I.e. def get_all_scene_datas(img: AICSImage, in_memory: bool) -> List[Tuple[str, xr.DataArray]]:
def open_scene(index: int) -> xr.DataArray:
# your existing function here
return data
scene_and_data = []
for i, scene in enumerate(img.scenes):
scene_and_data.append((f"{i} -- {scene}", open_scene(index=i))
return scene_and_data
# then have a function that just uses this list of tuples Basically split up the "naming and data getter" from the QT viewer stuff. That way you can just test the naming and data stuff!
No worries 🙂 You can actually keep working on your original branch and just push new commits. This PR will automatically pull in the changes. |
Ok, this error is related to the QtWidget. The requirements: Line 45 in 8f298c6
of just napari means no Qt. So I think based on napari docs ("Specifying a GUI Backend") that the proper thing here is to use napari[all] which will use the default GUI library—at the moment pyqt5.
|
I've got
I'm 99% sure this is due me having AICSImageIO 4.1.0 where you fixed the LIF scale issue: Edit: LAS X confirms it's the test that is wrong. Edit2: I've gotten a test working that checks for the presence of a widget for single and multi scene test files. If the length is not 0, then it confirms the widget name. I'll try to actually load an image layer to verify dtype, shape—hopefully this evening. |
Ahhh yes! Thanks for updating the Lif checks. Those were out-of-date. |
I don't know how it's built, but in pricinple you don't need an M1 Mac. arm64 and universal binaries can be built cross-architecture, at least using cmake: |
Codecov Report
@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 74.54% 79.14% +4.59%
==========================================
Files 7 7
Lines 110 163 +53
==========================================
+ Hits 82 129 +47
- Misses 28 34 +6
Continue to review full report at Codecov.
|
Thanks to @tlambert03 catching my bug, the test for selecting a scene from the widget should be working now. Passes for me locally anyways! |
Yep. Looks like there is just lint errors left. |
I've been working on the lint errors. I'm not sure how to deal with this one:
This is related to the Likewise, not sure what to do here:
I think this is regarding:
But I'm not sure what I should put here. |
this is because the # put this in the imports: from typing import TYPE_CHECKING
if TYPE_CHECKING:
from napari.types import LayerData, ReaderFunction, PathLike
make_napari_viewer: Callable[..., napari.Viewer] |
Am I doing this right?
Edit2: Nope! That broke everything. Edit3: Wowza, after reverting everything stayed broken, even when using a different conda env. Ended up uninstalling/reinstalling the plugin! Edit: for this one |
Yep! |
Hah. Oof. Give those: isort napari_aicsimageio/
black napari_aicsimageio/
flake8 napari_aicsimageio/ commands a run. |
Looks like the only things failing are just ubuntu tests which is somewhat expected because the ubuntu machines probably have odd display settings. I would be okay with adding a for the scene widget tests add something like: import platform
@pytest.mark.skipif(platform.system() == "Linux", reason="Ubuntu (Linux) runners abort for display tests") unless @tlambert03 has recommendations on how to get display tests working on GH Actions? Note the |
try changing the corresponding steps in the tests here to this: steps:
# add this step somewhere
- uses: tlambert03/setup-qt-libs@v1
# then use xvfb to run the test
- name: Test with pytest
uses: GabrielBB/xvfb-action@v1
with:
run: pytest --cov-report xml --cov=napari_aicsimageio napari_aicsimageio/tests/
|
maybe just use pre-commit. You could get the initial configuration from napari https://github.com/napari/napari/blob/main/.pre-commit-config.yaml |
Hey @psobolewskiPhD mind if I finish up this PR today? I believe I can push to your branch, and if not, I will clone your branch and work off of it. Hoping to get this in and release a new version today 👍 |
@JacksonMaxfield Sure. I was looking at doing this tonight: I think Talley's suggestion for the tests on Linux seems worth giving a whirl too: So I was going to do that as well—after the type stuff, so it could be easily reverted. But you're more then welcome to take the reins—I need to eat some diner and will then have time. With luck, it's quick, but you've seen my work to know better 🤣 |
Sounds good. Take the night off. Hopefully by tomorrow you should see a new release of |
The type checking change from @Czaki plus option 2 from Talley work on my end. Edit: I remembered to run |
❤️ |
Closing as I just merged #27 |
This PR adds support for selecting scenes from multi-scene files (such as LIF, CZI).
When a multi-scene files is opened or drag-n-dropped, a list widget opens on the right with all the scenes. Selecting a scene opens the scene in an image layer.
Napari-aicsimageio_trim.mov
Some commentary:
make build
testing suite work, due to pip/architecture issues (M1 arm64 Mac).reader
plugin type. Essentially,reader
plugins should deal with I/O and not UI. They don't have access to the Viewer. See this issue Provide access to Viewer instance to all hook functions napari/napari#2202 by @jwindhageracceptable
for such a flagship napari plugin—which I totally understand—but I went ahead and implemented that solution here.readlif
new release provides access to improved scene names..setCurrentRow()
Pull request recommendations: