convert cell_to_cellpar from ase's numpy implementation to pytorch #306
Conversation
There was a problem hiding this comment.
LGTM, test is good. Maybe get final sign off from @thomasloux
tests/models/test_orb.py
Outdated
| ) | ||
|
|
||
|
|
||
| def test_cell_to_cellpar(si_sim_state: SimState) -> None: |
There was a problem hiding this comment.
You can't use this one as it's cubic cell. So you wouldn't identify the possible issue of convention.
| lattice_per_system = torch.from_numpy( | ||
| cell_to_cellpar(cell_per_system.squeeze(0).cpu().numpy()) | ||
| ) | ||
| lattice_per_system = cell_to_cellpar(cell_per_system.squeeze(0)) |
There was a problem hiding this comment.
Ok I didn't see that the cell was extracted explicitly with the convention row_vector. When I say that the convention is not the same is that if A=Ase.atoms.cell[:] and B=SimState.cell, then A=B.T (transpose form). So in here, either you assume that the input is row and then you should have the same behaviour as ASE.cell_to_cellpar or you change that and then you have to change the input.
Given the current state of code, I suggest to use the same cell_to_cellpar convention as ASE, which is to assume that the input cell is a row_vector. Then modify the test accordingly
There was a problem hiding this comment.
Hi, I have changed the function to use the row convention and also updated the test.
3 participants
Summary
resolves #304
Checklist
Before a pull request can be merged, the following items must be checked:
We highly recommended installing the
prekhooks running in CI locally to speedup the development process. Simply runpip install prek && prek installto install the hooks which will check your code before each commit.