Skip to content

Remove squeeze to keep consistent shape#158

Open
thomasloux wants to merge 1 commit intoorbital-materials:mainfrom
thomasloux:fix/remove-squeeze-ts-orbmodel
Open

Remove squeeze to keep consistent shape#158
thomasloux wants to merge 1 commit intoorbital-materials:mainfrom
thomasloux:fix/remove-squeeze-ts-orbmodel

Conversation

@thomasloux
Copy link
Copy Markdown
Contributor

Squeeze can create issue of inconsistent shape.

Pratical use case:
variable cell relaxation of HCP titanium. It has only one site, so force would be reshape from [1, 3] to [3].

Copy link
Copy Markdown
Contributor

@benrhodes26 benrhodes26 left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR.

I am wary that shape logic can depend on the quantity of interest (energy/forces/stress). For instance, the ase calculator squeezes stress but not forces.

It looks like TorchSim are not clear in their documentation. So I'll wait until that is resolved until we merge this PR.

And once we do have confirmation from TorchSim, we should add a unit test here so we don't have a regression in the future

@CompRhys
Copy link
Copy Markdown
Contributor

The torchsim interface should be defined by this validation function and equivalent tests that we have on all models wrapped in main repo. What I need to look into more is why our test on the orb interface doesn't error if this is an issue here. Is it that this is a single atom motif? We should extend our tests to capture this edge case.

https://github.com/TorchSim/torch-sim/blob/ac2f18b8a9c8eaf08c0fc58f0038a6e7127314a3/torch_sim/models/interface.py#L204

https://github.com/TorchSim/torch-sim/blob/ac2f18b8a9c8eaf08c0fc58f0038a6e7127314a3/tests/models/conftest.py#L78

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.

3 participants