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

Add FaCT Finetuning for SAM #682

Merged
merged 18 commits into from
Sep 17, 2024
Merged

Add FaCT Finetuning for SAM #682

merged 18 commits into from
Sep 17, 2024

Conversation

anwai98
Copy link
Contributor

@anwai98 anwai98 commented Sep 5, 2024

Hi @caroteu,

I moved your fork's branch to micro-sam repo with all your changes in place.

I'll review the PR soon (also check out your discussions from the other PR) and ping you here!. Thanks!

@anwai98 anwai98 mentioned this pull request Sep 5, 2024
micro_sam/models/peft_sam.py Outdated Show resolved Hide resolved
micro_sam/training/util.py Outdated Show resolved Hide resolved
@anwai98
Copy link
Contributor Author

anwai98 commented Sep 17, 2024

Hi @constantinpape,

The FacT PR is good to go from our side (thanks to @caroteu). We have added an additional test for FacT method. We are working on doing some regression tests to understand a few minor details, but overall the method is good for another pair of eyes to review. Let us know if you spot something.

PS. We made a slight design change in get_sam_model (and linked functions) in favor of PEFT's usability.

Copy link
Contributor

@constantinpape constantinpape 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 overall. Just one minor comment about the tests.

test/test_models/test_peft_sam.py Outdated Show resolved Hide resolved
@constantinpape constantinpape merged commit a9fbe42 into dev Sep 17, 2024
3 checks passed
@constantinpape constantinpape deleted the fact_sam branch September 17, 2024 12:58
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.

3 participants