-
Notifications
You must be signed in to change notification settings - Fork 3
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug in scripted SOAP-BPNN without LayerNorm #166
Conversation
da4b3c9
to
18c40b5
Compare
system = ase.Atoms( | ||
"OHCN", | ||
positions=[[0.0, 0.0, 0.0], [0.0, 0.0, 1.0], [0.0, 0.0, 2.0], [0.0, 0.0, 3.0]], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
system = System(
species=torch.tensor([8, 1, 6, 7],
positions=torch.tensor([[0.0, 0.0, 0.0], [0.0, 0.0, 1.0], [0.0, 0.0, 2.0], [0.0, 0.0, 3.0]])
cell=torch.zeros((3, 3)),
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks fine since all the other tests also take in systems from ase.Atoms
, but if it's a matter of dropping more dependencies for the test, then I'm for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we did the same hot fix in llpr branch so we know it gets the job done. Leaving only a comment so that @frostedoyster has time to make adjustments to the test script as needed.
system = ase.Atoms( | ||
"OHCN", | ||
positions=[[0.0, 0.0, 0.0], [0.0, 0.0, 1.0], [0.0, 0.0, 2.0], [0.0, 0.0, 3.0]], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks fine since all the other tests also take in systems from ase.Atoms
, but if it's a matter of dropping more dependencies for the test, then I'm for it.
At the moment, we're using
torch.nn.Identity
instead of our LayerNorm if LayerNorm is disabled. This is fine, however, the torchscripted version oftorch.nn.Identity
only acceptstorch.Tensor
s, and not other objects馃摎 Documentation preview 馃摎: https://metatensor-models--166.org.readthedocs.build/en/166/