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 layer filtering #7

Merged
merged 6 commits into from
Aug 8, 2024

Conversation

jo-mueller
Copy link
Contributor

Fixes #5

Minor change that checks whether a selected layer in the viewer for fusion is actually an image or a label. I used a convenience function to retrieve the layers of the correct kind which is then called in both the load all and load selected functions:

    def _get_compatible_layers(self, only_selected=False):
        if only_selected:
            return [l for l in self.viewer.layers.selection if isinstance(l, (Image, Labels))]
        else:
            return [l for l in self.viewer.layers if isinstance(l, (Image, Labels))]

I also added some warnings in case an invalid selection is passed. Let me know what you think :)

- Add a check in `load_layers` method to filter out non-image and non-label layers.
- Update `load_layers` method to add only image and label layers to `self.input_layers`.
- Update `run_registration` and `run_fusion` methods to use `self.input_layers` with only image and label layers.
- Add tests to check if non-image layers are filtered out in `load_layers` method.
- Add tests to check if `Fuse/Register` buttons are not grayed out when only image and label layers are selected.
Copy link
Collaborator

@m-albert m-albert left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🙏

I like having a helper function as in the future there'll be other plugin widgets that'll need to do the same compatibility check.

Suggestion: Since loading always goes through the bottleneck of load_layers, I think the cleanest would be to do the filtering only there (currently there's some redundancy, see the comment in the code review).

src/napari_stitcher/_stitcher_widget.py Outdated Show resolved Hide resolved
@jo-mueller
Copy link
Contributor Author

Hi @m-albert ,

thanks for the review, that's indeed the better place to put it. I have moved the filtering there with the slight difference that I filter the input layer variable rather than self.input_layers as the former is used further down in the loading and the plugin seems to run into trouble when layers and input_layers get out of sync.

I also removed the convenience function again since it is right now only used in this one place - one can re-introduce it if there is a need for it down the road :)

@m-albert
Copy link
Collaborator

m-albert commented Aug 8, 2024

Looks great! Thanks a lot for this iteration.

I have moved the filtering there with the slight difference that I filter the input layer variable rather than self.input_layers as the former is used further down in the loading and the plugin seems to run into trouble when layers and input_layers get out of sync.

Completely agreed that only filtered/confirmed layers should be included in self.input_layers. Indeed there are callbacks on added layers monitoring for changes in transformations after adding them to the widget.

I also removed the convenience function again since it is right now only used in this one place - one can re-introduce it if there is a need for it down the road :)

Sounds good.

Cool, merging this now. Thanks a lot for contributing!

@m-albert m-albert merged commit 42d8130 into multiview-stitcher:main Aug 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Register/Fuse buttons grayed out if one layer is not Image
2 participants