Validate multihit branch lengths #124
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
The issues that ended up causing the discrepancy were:
|
willdumm
commented
Apr 23, 2025
willdumm
commented
Apr 23, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR implements changes to validate multihit branch lengths and improve branch length optimization along with related refactoring. Key changes include:
- Addition of new utility functions (flatten_codon_idxs, unflatten_codon_idxs, nt_idx_tensor_of_str) and corresponding tests.
- Refactored optimization routines in molevol.py using SciPy’s bracket and minimize_scalar functions.
- Consistent updating of branch length handling and multihit correction across modules (multihit, dxsm, dnsm, ddsm, dasm, framework).
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sequences.py | Added tests for new codon index conversion functions. |
| setup.py | Added the scipy dependency. |
| netam/sequences.py | Introduced new codon indexing utilities supporting multihit workflows. |
| netam/multihit.py | Updated HitClassDataset to subclass BranchLengthDataset and refactored branch length handling. |
| netam/molevol.py | Overhauled optimization functions and added new codon reshaping utilities. |
| netam/models.py | Updated forward docstring to reflect new input expectations. |
| netam/hit_class.py | Modified multihit correction using the new normalization routine. |
| netam/framework.py | Added parallel branch length optimization with multiprocessing. |
| netam/dxsm.py, dnsm.py, ddsm.py, dasm.py | Replaced deprecated branch length references with the updated ones. |
matsen
approved these changes
Apr 24, 2025
netam/dasm.py
Outdated
|
|
||
| # We have to clamp the predictions to avoid log(0) issues. | ||
| preds = torch.clamp(preds, min=torch.finfo(preds.dtype).eps) | ||
| preds = torch.clamp(preds, min=torch.finfo(preds.dtype).tiny) |
Contributor
Author
|
Look how wrong the branch lengths were! (I think they were more wrong with multihit adjustment, but we can't make that comparison because we also changed how the multihit model gets applied) These are on 50k pcps from the Jaffe heavy chain dataset. |
add branch length tests remove old comment update comment add lots of tests maybe fixed? fix multihit normalization and add tests tweaks to branch lengths finalized branch length optimization fixed tests again cleanup fix copilot suggestion dasm training performs well cleanup and fix tests format lint remove unnecessary warning
willdumm
force-pushed
the
123-multihit-branch-lengths
branch
from
0cec219 to
bb412a1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #123 for discussion, referencing a test in this PR