Add SSF method #727
Merged
Add SSF method #727
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes SSF implementation - scale and shift parameters now seem to be working as expected --------- Co-authored-by: Anwai Archit <[email protected]>
|
This seems to work now. I think we can merge this as our experiments are in place for this method. cc: @constantinpape |
constantinpape
approved these changes
Dec 14, 2024
|
I had to fix some merge conflicts, will merge once the tests pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Superseeds #726.
Hi @caroteu,
Thanks for adding the method.
I gave it a refactor so that we avoid adding components to
PEFT_Samand rely on surgery classes for implementing peft methods. And in general for PEFT methods in future, we should rely on performing all updates via surgery classes and keep the minimal design of the peft sam functionality.There's also a major issue anyways (observed this in your PR, tried to take a look at it, couldn't manage to find out what's going on), but the SSF method does not work atm (i.e. this can be easily checked with the change in parameters when we freeze the image encoder vs SSF - the parameter count is the same + there are no parameters which expect gradient updates, which is not supposed to be the case as we update the scale and shift parameters).
I suggest you take over and see if you can figure out the issue.
(In case you figure out something, create a PR from the remote at https://github.com/computational-cell-analytics/micro-sam to the branch
add-ssf- this would help us develop over PRs together)Let me know if you have any questions.