Perceiver interpolate position embedding #30979
Conversation
| inputs: torch.LongTensor, | ||
| pos: Optional[torch.Tensor] = None, | ||
| network_input_is_1d: bool = True, | ||
| interpolate_pos_encoding: bool = None, |
There was a problem hiding this comment.
The argument doesn't appear to be used in this method?
There was a problem hiding this comment.
When I ran the test, TextPreprocesor and AudioPreprocessor didn't fail but only MutimodalPreprocessor did so I missed those. Just added.
There was a problem hiding this comment.
interpolate_pos_encoding still isn't used in this method., or the one for PerceiverAudioPreprocessor. Is this for providing a consistent signature?
…nce only Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
g1y5x3
force-pushed
the
perceiver_inter_pos_embed
branch
from
9a55e50
to
2891e0d
Compare
|
Thanks for the review, I have addressed the comments. Turns out after working on the no longer needed |
| inputs: torch.LongTensor, | ||
| pos: Optional[torch.Tensor] = None, | ||
| network_input_is_1d: bool = True, | ||
| interpolate_pos_encoding: bool = None, |
There was a problem hiding this comment.
interpolate_pos_encoding still isn't used in this method., or the one for PerceiverAudioPreprocessor. Is this for providing a consistent signature?
| if self.input_preprocessor is not None: | ||
| inputs, modality_sizes, inputs_without_pos = self.input_preprocessor(inputs) | ||
| inputs, modality_sizes, inputs_without_pos = self.input_preprocessor( | ||
| inputs, interpolate_pos_encoding=interpolate_pos_encoding | ||
| ) |
There was a problem hiding this comment.
@amyeroberts since self.input_processor here could also be either TextPreprocessor or MultimodalPreprosessor, it would complain got unexpected keyword argument if it was not added to the forward() of those two corresponding methods. Alternatively, maybe we could add another condition here to call self.input_processor with just two arguments unless it is ImagePreprocessor?
There was a problem hiding this comment.
No, that's OK, I just wanted to make sure that this was deliberate
* add test that currently fails * test passed * all perceiver passed * fixup, style, quality, repo-consistency, all passed * Apply suggestions from code review: default to False + compute sqrt once only Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * fix a minor bracket * replace dim with self._num_channels * add arguments to the rest preprocessors --------- Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
* add test that currently fails * test passed * all perceiver passed * fixup, style, quality, repo-consistency, all passed * Apply suggestions from code review: default to False + compute sqrt once only Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * fix a minor bracket * replace dim with self._num_channels * add arguments to the rest preprocessors --------- Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
What does this PR do?
Add interpolate position embedding to Perceiver #30579. Not really sure how to test the correctness of output value since it does change the entire forward pass. However, correctness of output shape is pretty straight forward.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@amyeroberts