Torsion angle loss #81
Conversation
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/beignet/nn/functional/_torsion_angle_loss.py:7
- [nitpick] The parameter name 'input' shadows the built-in Python function. Consider renaming it to 'input_tensor' for improved clarity.
def torsion_angle_loss(input, target: Tuple[Tensor, Tensor]) -> Tensor:
src/beignet/nn/functional/_torsion_angle_loss.py:19
- Instead of using torch.unsqueeze, consider using torch.norm with keepdim=True (e.g. torch.norm(input, dim=-1, keepdim=True)) to improve code readability.
a = input / torch.unsqueeze(torch.norm(input, dim=-1), dim=-1)
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
tests/beignet/nn/functional/test__torsion_angle_loss.py:6
- [nitpick] Consider renaming the variable 'input' as it shadows the built-in function 'input'.
input = torch.ones([1, 1, 7, 2])
src/beignet/nn/functional/_torsion_angle_loss.py:7
- [nitpick] Consider renaming the parameter 'input' to avoid shadowing the built-in and to improve clarity.
def torsion_angle_loss(input, target: Tuple[Tensor, Tensor]) -> Tensor:
| import torch | ||
|
|
||
|
|
||
| def test_torsion_angle_loss(): |
There was a problem hiding this comment.
@kleinhenz Just a smoke test.
I will revisit with improved unit tests after I implement the scaffolding for all the losses.
| ------- | ||
|
|
||
| """ | ||
| a = input / torch.norm(input, dim=-1, keepdim=True) |
There was a problem hiding this comment.
Should this be clamped? 🤔
| from torch import Tensor | ||
|
|
||
|
|
||
| def torsion_angle_loss(input, target: Tuple[Tensor, Tensor]) -> Tensor: |
There was a problem hiding this comment.
Can we call this something like alphafold2_torsion_angle_loss and add a reference to alphafold2 supplement algorithm 27 in the docstring?
There was a problem hiding this comment.
Of course. I will add a docstring!
As far as renaming, are there multiple torsion angle losses?
There was a problem hiding this comment.
I think you can imagine doing some different things. If you predict an angle instead of a 2d vector then the anglenorm part of this doesn't make sense and you would just do cosine of the difference. You could also do something like nll of wrapped normal.
| @@ -0,0 +1,36 @@ | |||
| from typing import Tuple | |||
There was a problem hiding this comment.
We could use PEP 585 instead so the hint would be tuple[Tensor, Tensor]. I find it slightly cleaner but not a big deal either way.
There was a problem hiding this comment.
Looking into this more it looks like those hints are actually being deprecated and removed once python 3.9 is EOL
|
|
||
| b, c = target | ||
|
|
||
| x = torch.mean( |
There was a problem hiding this comment.
Can we call x and y something slightly more descriptive like loss_torsion and loss_anglenorm to make it easier to compare with the description in the supplement?
Closes #66