Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Download models with pooch #276
Download models with pooch #276
Changes from 4 commits
00006fc
926b4ba
09bc898
e5b1433
8460954
c00c660
0387b95
32c8bb2
1c29685
865d613
a7a19b9
3b40099
355600f
ce52b29
0823237
c1081b7
22f9274
4d7a406
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 134 in micro_sam/precompute_state.py
Codecov / codecov/patch
micro_sam/precompute_state.py#L134
Check warning on line 72 in micro_sam/sam_annotator/_widgets.py
Codecov / codecov/patch
micro_sam/sam_annotator/_widgets.py#L72
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.
I think we should keep this. Merging this into
get_custom_sam_model
would be confusing and I don't see a big issue keeping it here. See my main comment for more details.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.
Ok. I have restored the
checkpoint_path
keyword argument.I will need you to test this (because all the example scripts involving checkpoint_path rely on data you personally have). The
_get_checkpoint
function is not covered by any of the existing tests, so it's safe to say we're not testing anything involvingcheckpoint_path
right now.Could you please run:
examples/annotator_with_custom_model.py
examples/finetuning/use_finetuned_model.py
examples/use_as_library/instance_segmentation.py
It would be great if we could upload the data and custom weights for
examples/annotator_with_custom_model.py
to zenodo or similar - or even make a similar example but with smaller 2d data and model weights. Ideally then anybody could run this example as a test.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.
Hi @GenevieveBuckley,
the models are all available via zenodo already. See https://github.com/computational-cell-analytics/micro-sam/blob/master/micro_sam/util.py#L46-L49. (I agree though that we should list this in the doc, I have added that to #249)
and I changed the examples in #280 so that all the examples use sample data.
I don't think that's true either since merging #280. This now uses a custom checkpoint. But you would need to merge the current
dev
branch in here first to get those tests.I will still go ahead and test the examples with this branch.
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.
We can just add something like this to support
checkpoint_path
: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.
So I don't see any problem with mixing up hashes.
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.
I'm confused - if the weights are different, the hash value for the object should be different too, right? So how can anybody possibly do some extra fine tuning on a particular model, save the new weights, and then use checkpoint_path and give it the hash value stored in the pooch registry?
I might misunderstand what is actually happening with
checkpoint_path
.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.
Sorry I should have written a better description here. I will try to explain what happens if a
checkpoint_path
is passed or not:checkpoint_path
is given: this is a local path to sam weigths. E.g. if a user has fine-tuned on their own data. In this case we don't download any weights but initialize the model from these local weights. We do not check the weight file against any hashes.checkpoint_path
is not given: we download the SAM weights corresponding tomodel_type
. (I agree that this name is confusing, see comment below). We can check their hash.When finetuning (or passing any other weights via
checkpoint_path
) we don't check the weights against the hash values in the pooch registry. From what I understand the current code is doing exactly what I described, since we only use the model registry in the case wherecheckpoint_path is None
, and only then we actually check the hashes.