-
Notifications
You must be signed in to change notification settings - Fork 7
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
Abstracting prompting transformer for use in L2P and S-Prompt #420
Conversation
Coverage reportNote Coverage evolution disabled because this PR targets a different branch The coverage rate is
Diff Coverage details (click to unfold)src/renate/benchmark/models/l2p.py
src/renate/updaters/experimental/l2p.py
|
src/renate/benchmark/models/l2p.py
Outdated
) | ||
# text transformers dont support cls_feat. | ||
else: | ||
if self._is_text_transformer: |
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 flatten the if else(if else) part to if, elif, else
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.
Fixed in d8ef1c8
prompter = PromptPool( | ||
embedding_dim=transformer._embedding_size, | ||
embedding_dim=transformer.transformer._embedding_size, |
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.
in the following, we access a lot of protected attributes of the transformer. do we want to keep it that way or rather make them public?
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.
Fixed in 3896135
src/renate/benchmark/models/l2p.py
Outdated
class PromptedTransformer(nn.Module): | ||
"""This generic module is the basic prompted transformer. It takes in a model string and creates | ||
the appropriate transformer. If not prompted, it returns features, and if prompted, it returns | ||
the full feature using those prompts and the input image/text. |
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.
is this description still accurate? I don't see the input being returned. Maybe clarify difference between features and full feature. Without context, it might not even be clear that this is the model output.
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.
Fixed in 3896135.
The previous version of L2P had the "prompting" mechanism tightly coupled. This PR separates the Prompting and the prompt selection strategy.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.