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

BioImage.IO Core: Improve interface for PlantSeg models #248

Merged
merged 6 commits into from
May 28, 2024

Conversation

qin-yu
Copy link
Collaborator

@qin-yu qin-yu commented May 26, 2024

Changes

This PR improves #247

  • Improve UI: Change "Single Patch" to "Batch Size" and make auto-batch-size-discover the default

    image

  • Choose to show PlantSeg or all models in dropdown list from BioImage.IO Model Zoo

    image

  • Remove PlantSeg model zoo filters for BioImage.IO models

    image

Before this PR

image

@qin-yu qin-yu self-assigned this May 26, 2024
@qin-yu qin-yu added enhancement New feature or request BioImage.IO Related to BioImage.IO and AI4Life GUI Napari GUI related labels May 26, 2024
@lorenzocerrone
Copy link
Collaborator

Looks very nice! :) It's much cleaner.

Since we are discussing this widget, I have two more ideas/suggestions:

  • Is there ever a need to manually adjust the patch_halo for a standard user? To me, the default always worked the best. Maybe we should also remove it?
  • The Single Patch checkbox may be cryptic. What could be a better alternative? Maybe "Single Patch (Lower Memory Usage)"

`AliasChoices` can only be used in `validation_alias`; `alias` takes only `str`
@qin-yu
Copy link
Collaborator Author

qin-yu commented May 26, 2024

  • Is there ever a need to manually adjust the patch_halo for a standard user? To me, the default always worked the best. Maybe we should also remove it?

I originally planned to implement automatic halo sizing when I noticed that the halo size was set to a constant value. However, I shifted focus to correct the halo implementation in PlantSeg and pytorch-3dunet, and the initial plan was set aside. I’ll proceed based on this paper: https://nvlpubs.nist.gov/nistpubs/jres/126/jres.126.009.pdf. My belief is that group norm and wrong halo implementation (and perhaps constant halo size) worked in previous settings because the images were always similar. Now I trained new official models with batch norm that reduce hallucination, fixed halo padding that reduce tiling artefact (it's not too wrong to claim we "removed tiling artefact"). The next step is to automatically set a minimal halo size based on the architecture so that the prediction is as exact as theoretically possible and completely removes tiling artefact.

In the end, yes we remove the user-input halo, but do not use a constant. What do you think?

  • The Single Patch checkbox may be cryptic. What could be a better alternative? Maybe "Single Patch (Lower Memory Usage)"

Thanks for the advice, I'll fix it tomorrow!

@lorenzocerrone
Copy link
Collaborator

Cool paper, a great work retraining the moodels! :) I like the idea of havig the perfect halo size calculated from the architecture. My point is just that exposing the halo parameter to the users is probably more confusing than helpful.

Current models on BioImage.IO Model Zoo were uploaded before implementation of BioImage.IO Spec format version 0.5, but Spec v0.5 is in use already.
@qin-yu
Copy link
Collaborator Author

qin-yu commented May 27, 2024

  • The Single Patch checkbox may be cryptic. What could be a better alternative? Maybe "Single Patch (Lower Memory Usage)"

Hey Lorenzo, I don't actually understand the significance of having "Single Patch". If the model runs on host then PlantSeg finds batch size 1, otherwise finds the best batch size. I guess we can remove it and always find the best batch size for users?

But I made the changes you requested.

Update

There is one use case though: when someone has only one graphics card and plan to run PlantSeg for many images while using the same card. Yes, let's keep it.

@qin-yu qin-yu marked this pull request as ready for review May 27, 2024 22:11
- Improve readability
- Make multi-patch mode the default
@lorenzocerrone
Copy link
Collaborator

  • The Single Patch checkbox may be cryptic. What could be a better alternative? Maybe "Single Patch (Lower Memory Usage)"

Hey Lorenzo, I don't actually understand the significance of having "Single Patch". If the model runs on host then PlantSeg finds batch size 1, otherwise finds the best batch size. I guess we can remove it and always find the best batch size for users?

But I made the changes you requested.

Update

There is one use case though: when someone has only one graphics card and plan to run PlantSeg for many images while using the same card. Yes, let's keep it.

Yeah, that's exactly the use case. In general, if you have a single graphic card, it's nicer to keep a bit of Slack for other apps.

PREDICTION_MODES = (PREDICTION_MODE_P, PREDICTION_MODE_B) # PREDICTION_MODES will not be binary, thus not boolean

BIOIMAGEIO_FILTER = [("PlantSeg Only", True), ("All", False)]
SINGLE_PATCH_MODE = [("Find Batch Size", False), ("Single Patch", True)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since "batch size" is also the label, should we use simply:

SINGLE_PATCH_MODE = [("Auto", False), ("One", True)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then we have space to put "(Lower VRAM usage)"/"(Lower memory usage)", we add this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

SINGLE_PATCH_MODE = [("Auto", False), ("One (Lower VRAM Usage)", True)]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like this:

image

Copy link
Collaborator

@lorenzocerrone lorenzocerrone left a comment

Choose a reason for hiding this comment

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

Looks good!

@qin-yu qin-yu merged commit ef52b54 into master May 28, 2024
1 check passed
@qin-yu qin-yu deleted the qy/bioimage-io-core branch May 31, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BioImage.IO Related to BioImage.IO and AI4Life enhancement New feature or request GUI Napari GUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants