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

Refactor the heaviest of lifters #34

Closed
talmo opened this issue Apr 30, 2024 · 1 comment
Closed

Refactor the heaviest of lifters #34

talmo opened this issue Apr 30, 2024 · 1 comment

Comments

@talmo
Copy link
Contributor

talmo commented Apr 30, 2024

This function gives me nightmares:

https://github.com/talmolab/biogtr/blob/a47c48b84535632290ab7757d65abbfd787d69bc/biogtr/models/transformer.py#L444-L483

This is especially salient due to the fact that this is so deep in the call stack (TransformerDecoderLayer <- TransformerDecoder <- Transformer <- GlobalTrackingTransformer <- GTRRunner <- ...).

This induces so many code paths it's nearly impossible to trace:

https://github.com/talmolab/biogtr/blob/a47c48b84535632290ab7757d65abbfd787d69bc/biogtr/models/transformer.py#L457-L458

https://github.com/talmolab/biogtr/blob/a47c48b84535632290ab7757d65abbfd787d69bc/biogtr/models/transformer.py#L460

Why??? It's just for self-attention, so why not just keep it in that conditional a few lines down??

https://github.com/talmolab/biogtr/blob/a47c48b84535632290ab7757d65abbfd787d69bc/biogtr/models/transformer.py#L462-L465

And surely we can come up with better names than tgt, tgt2, tgt3??

https://github.com/talmolab/biogtr/blob/a47c48b84535632290ab7757d65abbfd787d69bc/biogtr/models/transformer.py#L467-L481


Both here and in the upstream Transformer.call, let's fix the variable names, disambiguate "target"/tgt, memory, and canonical key/query/value nomenclature.

@aaprasad
Copy link
Contributor

aaprasad commented May 3, 2024

Done in #36

@aaprasad aaprasad closed this as completed May 3, 2024
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