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

Split forward functions in DXSM models #74

Merged
merged 6 commits into from
Oct 28, 2024
Merged

Conversation

willdumm
Copy link
Contributor

@willdumm willdumm commented Oct 25, 2024

Splits forward functions into represent and predict methods, where represent does everything except applying the final linear layer, in preparation for https://github.com/matsengrp/dnsm-experiments-1/issues/28

Will close #72.

Will need to be rebased on #73 when that's done/merged.

  • Are the modified models all the ones that are used in DXSM? It looks like CNNModel forward functions would also accept a similar split. Do we want them split, too?

Copy link
Contributor

@matsen matsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; minor suggestions here.

netam/models.py Outdated
representation: A tensor of shape (B, L, E) representing the
embedded parent sequences.
Returns:
A tensor of shape (B, L, 1) representing the log level of selection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare with (B, L, 20) below

netam/models.py Outdated
mask: A tensor of shape (B, L) representing the mask of valid amino acid sites.

Returns:
A tensor of shape (B, L, 20) representing the log level of selection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rather than 20 it should be out_features which is a kwd arg. It's set to 1 for classic DSNM.

Speaking of which, in the definition of the hyperparameters we have

           "output_dim": self.linear.out_features,

We should decide on one or the other of dim or features. What do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively I'd say dim, but maybe features is a more torch-y way of saying it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beh. Let's honor your intuition.

Makefile Outdated
@@ -21,6 +21,9 @@ lint:
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --max-complexity=30 --max-line-length=127 --statistics --exclude=_ignore

lintless:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to make this the default lint if you'd like.

@willdumm willdumm force-pushed the 72-split-dnxm-forward branch from 9281294 to 2992dee Compare October 28, 2024 19:11
@willdumm willdumm merged commit dda7987 into main Oct 28, 2024
1 check passed
@willdumm willdumm deleted the 72-split-dnxm-forward branch October 28, 2024 21:18
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

Successfully merging this pull request may close these issues.

Split forward functions for DNXM models into two functions
2 participants