Fix unit and frechet cell FIRE optimizers not rescaling atom positions after each cell update#199
Merged
Fix unit and frechet cell FIRE optimizers not rescaling atom positions after each cell update#199
Conversation
by adding temp variable old_row_vector_cell to store the prev state of row_vector_cell and use it to scale atomic positions with torch.bmm(inv_old_cell_batch, current_new_row_vector_cell) after each cell update
…_diff now that we get better torch-sim ASE agreement
…nit Cell FIRE optimizers in test_optimizers_vs_ase.py - new fixtures for initial SimState of rhombohedral OsN2 and distorted FCC Al - new tests comparing torch-sim's Frechet Cell FIRE and Unit Cell FIRE optimizers with ASE's implementations
janosh
added
tests
tests/test_optimizers_vs_ase.py
Outdated
| "osn2_sim_state", | ||
| "frechet", | ||
| FrechetCellFilter, | ||
| 50, |
Collaborator
There was a problem hiding this comment.
Should we track different nsteps? i.e compare for 20, 50, 100 etc and see if they give same structure and properties.
Collaborator
Author
There was a problem hiding this comment.
good call, see b2d4f15 which makes tests in test_optimizers_vs_ase.py more stringent by comparing ASE and torch-sim EFS + cell at multiple checkpoints in each relaxation
Collaborator
There was a problem hiding this comment.
That's looks good!
… by comparing ASE and torch-sim EFS + cell at multiple checkpoints during each relaxation - _run_and_compare_optimizers now accepts a list of checkpoints instead of a single n_steps parameter
CompRhys
reviewed
May 16, 2025
even though passing locally
CompRhys
reviewed
May 16, 2025
distorted_fcc_al_conventional_sim_state fixtures to root conftest
abhijeetgangan
approved these changes
May 16, 2025
Contributor
|
Thanks a lot for your quick fix! 🙏 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
closes #198
torch_sim/optimizers.pyhad a bug discovered by @t-reents in FIRE relaxation where atomic positions were not correctly updated when the cell changed. resolved by scaling atomic positions by the difference between new and previous cell after each cell updateOsN2and distortedFCC Alstructures intest_optimizers_vs_ase.py, covering both Frechet and Unit Cell FIRE optimizerstest_optimizers_vs_ase.pydue to improved agreement betweentorch-simandASEoptimizersin a future PR, it probably makes sense to change the handling of atom positions from absolute to fractional (unwrapped) coordinates which would mean the atom positions don't require manual scaling after cell changes