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

Prototype for annotator plugins #304

Merged
merged 22 commits into from
Jan 18, 2024
Merged

Prototype for annotator plugins #304

merged 22 commits into from
Jan 18, 2024

Conversation

constantinpape
Copy link
Contributor

Minimal prototype for annotator plugin.

This is an attempt to refactor the current implementations of annotation tools into proper napari plugins.
It contains a minimal working example for the 2d annotation tool (not yet all features added).

Design

  • The base class sam_annotator._annotator._AnnotatorBase initializes all shared functionality for the annotation plugins.
  • The annotation tools inherit from this class and add their specific functionality, e.g. sam_annotator.annotator.Annotator2d.
    • (There we could also implement a function as entry point from python / the CLI so that the tool can be started without going through the napari plugin interface).
  • In addition, all widgets will be refactored to _widgets.py, and some more clean up.

This would address #200 and #206.

@constantinpape constantinpape changed the base branch from master to dev January 4, 2024 21:04
@constantinpape
Copy link
Contributor Author

Need to update test/test_gui.py to match these changes!

@constantinpape
Copy link
Contributor Author

I have now ported all annotation tools to the new plugin design and everything is working!
The overall design approach is summarized here: https://github.com/computational-cell-analytics/micro-sam/blob/b440dcb493502721ff99ba0f4be1077905d0e892/doc/development.md

Next steps:

  • Before merging this PR we will merge the current dev into master and make a 0.4.0 release (to publish the recent improvements to the training methodology).
  • The core plugin functionality in this PR is working, but there are a lot of smaller follow-ups to beautify the plugin and extend functionality. We will work on this for version 0.5.0.

Copy link
Collaborator

@GenevieveBuckley GenevieveBuckley left a comment

Choose a reason for hiding this comment

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

This class based creation of dock widgets is a bit more heavy duty that the approach I tried (involving a magicgui container stuffed with the existing widgets). I hope/think that will make it more robust.

I'm broadly supportive of this design. Note that I've mostly looked at the code, and haven't spent time playing with it in the GUI, eg: to check things like whether the embeddings don't get recomputed unnecessarily, or that computing embeddings, then changing the selected input image produces something sensible, etc.

We've discussed this previously, and I still agree that refactoring to reduce duplication (eg: combining the 2D and 3D annotation widgets, etc.) is a good long term goal. Not in this PR, but something later. I think that structure you've made with this design will probably help with that long term goal. It seems a little easier to understand how the pieces fit together here.

micro_sam/sam_annotator/_annotator.py Show resolved Hide resolved
micro_sam/sam_annotator/util.py Outdated Show resolved Hide resolved
examples/annotator_2d.py Show resolved Hide resolved
@constantinpape
Copy link
Contributor Author

constantinpape commented Jan 9, 2024

Thanks for the review @GenevieveBuckley !

I'm broadly supportive of this design. Note that I've mostly looked at the code, and haven't spent time playing with it in the GUI, eg: to check things like whether the embeddings don't get recomputed unnecessarily, or that computing embeddings, then changing the selected input image produces something sensible, etc.

I have tried each tool a couple of times and everything seems to work. Of course there might still be some more subtle bugs or issue (e.g. that everything is set correctly when updating the image etc.).
I will do some more testing of this later, and we will also test it more in the lab before doing an actual release.

We've discussed this previously, and I still agree that refactoring to reduce duplication (eg: combining the 2D and 3D annotation widgets, etc.) is a good long term goal. Not in this PR, but something later. I think that structure you've made with this design will probably help with that long term goal. It seems a little easier to understand how the pieces fit together here.

Yes, there's probably some more room to combine functionality here in follow up work.

Regarding your more in-detail review and next steps:

  • I have addressed the comments about the tracking util functions and the tests.
  • I will make dedicated issues out of TODOs when finalizing this PR (and open a few more follow up issues).
  • And I will do that as soon as we have made a new release with the updated training functionality and new fine-tuned models. (Or have merged the current dev into master as preparation for this, hopefully by the end of this week.)

@GenevieveBuckley
Copy link
Collaborator

I have tried each tool a couple of times and everything seems to work. Of course there might still be some more subtle bugs or issue (e.g. that everything is set correctly when updating the image etc.).

That's great! 😄 Yes, of course unexpected issues can always pop up later. It sounds like this is a good balance between doing enough real world testing to catch any obvious problems, but not too much that holds the project back.

@constantinpape constantinpape marked this pull request as ready for review January 17, 2024 22:29
@GenevieveBuckley
Copy link
Collaborator

I see this PR is now out of draft state. I don't have any substantive new comments to add since our last discussion.

@constantinpape
Copy link
Contributor Author

I see this PR is now out of draft state. I don't have any substantive new comments to add since our last discussion.

Thanks @GenevieveBuckley. I moved this out of draft because I have merged dev into master now and made a new release and wanted to wait with merging this one until that.
I will clean up and merge this PR today.

@constantinpape constantinpape merged commit 4aff57f into dev Jan 18, 2024
3 checks passed
@constantinpape constantinpape deleted the plugin-refactor branch January 18, 2024 09:38
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.

2 participants