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

Incorrectly implemented MultiHeadAttention in PseTae #40

Open
sbgeophd opened this issue Oct 5, 2021 · 2 comments
Open

Incorrectly implemented MultiHeadAttention in PseTae #40

sbgeophd opened this issue Oct 5, 2021 · 2 comments
Assignees

Comments

@sbgeophd
Copy link

sbgeophd commented Oct 5, 2021

The implementation of the PseTae MultiHeadAttention appears to have a mistake.

The original implementation applies 2 fully connected layers on the query tensor (see here: https://github.com/VSainteuf/pytorch-psetae/blob/master/models/tae.py#L133)

However, in the eo-flow implementation, while both fully connected layers are defined, the 2nd (defined here: https://github.com/sentinel-hub/eo-flow/blob/master/eoflow/models/pse_tae_layers.py#L50) is not used as would be expected here: https://github.com/sentinel-hub/eo-flow/blob/master/eoflow/models/pse_tae_layers.py#L66 (indeed, it is not used at all in the code).

@devisperessutti
Copy link
Contributor

Hi @sbgeophd !

Yes, you are correct, the second connected layer is not used. I can't recall if that was done on purpose or not, but I'd guess we just overlooked this part.

Do you have the capacity to make a pull request fixing this issue? If not, we could do it but it might not happen very soon.

Thanks for reporting the issue.

@devisperessutti devisperessutti self-assigned this Oct 6, 2021
@sbgeophd
Copy link
Author

sbgeophd commented Oct 6, 2021

I'm in theory happy to make a pull request, but while I think I've fixed it, the model still doesn't work for me. I'll submit a pull request if/when I get it working.

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

No branches or pull requests

2 participants