Directly compare ASE vs TorchSim Frechet cell FIRE relaxation #146
Conversation
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
orionarcher
reviewed
Apr 14, 2025
There was a problem hiding this comment.
Great check to have but I'd suggest making it a script in examples/scripts/7_Others. Otherwise it will require a not very generalizable change to the testing action to get it passing.
You can add the dependencies at the top to get the tests passing, like this:
# /// script
# dependencies = [
# "mace-torch>=0.3.11",
# ]
# ///
|
Agree with @orionarcher that it should be a script for now. Currently the tests have a lot of repeated code and could be made cleaner along with integration tests with ASE. |
orionarcher
force-pushed
the
main
branch
2 times, most recently
from
02be493 to
80a4802
Compare
janosh
reviewed
Apr 15, 2025
janosh
force-pushed
the
main
branch
3 times, most recently
from
2a6a24f to
926e25d
Compare
janosh
force-pushed
the
main
branch
3 times, most recently
from
a3b20fb to
e763da9
Compare
3 tasks
* update changelog for v0.2.0 * minor modification for PR template * formatting fixes * formatting and typos * remove contributors bc they aren't linked
CompRhys
force-pushed
the
optimizer_ase_test
branch
from
c2e2942 to
e17db82
Compare
…in orig FIRE paper) and improve coverage in `test_optimizers.py` (#174) * feat(fire-optimizer-changes) Update fire_step in optimizers.py based feature/neb-workflow * reset optimizers.py to main version prior to adding updated changes * (feat:fire-optimizer-changes) - Added ase_fire_step and renamed fire_step to vv_fire_step. Allowed for selection of md_flavor * (feat:fire-optimizer-changes) - lint check on optimizers.py with ruff * (feat:fire-optimizer-changes) - added test cases and example script in examples/scripts/7_Others/7.6_Compare_ASE_to_VV_FIRE.py * (feat:fire-optimizer-changes) - updated FireState, UnitCellFireState, and FrechetCellFireState to have md_flavor to select vv or ase. ASE currently coverges in 1/3 as long. test cases for all three FIRE schemes added to test_optimizers.py with both md_flavors * ruff auto format * minor refactor of 7.6_Compare_ASE_to_VV_FIRE.py * refactor optimizers.py: define MdFlavor type alias for SSoT on MD flavors * new optimizer tests: FIRE and UnitCellFIRE initialization with dictionary states, md_flavor validation, non-positive volume warnings brings optimizers.py test coverage up to 96% * cleanup test_optimizers.py: parameterize tests for FIRE and UnitCellFIRE initialization and batch consistency checks maintains same 96% coverage * refactor optimizers.py: consolidate vv_fire_step logic into a single _vv_fire_step function modified by functools.partial for different unit cell optimizations (unit/frechet/bare fire=no cell relax) - more concise and maintainable code * same as prev commit but for _ase_fire_step instead of _vv_fire_step * (feat:fire-optimizer-changes) - added references to ASE implementation of FIRE and a link to the original FIRE paper. * (feat:fire-optimizer-changes) switched md_flavor type from str to MdFlavor and set default to ase_fire_step * pytest.mark.xfail frechet_cell_fire with ase_fire flavor, reason: shows asymmetry in batched mode, batch 0 stalls * rename maxstep to max_step for consistent snake_case fix RuntimeError: a leaf Variable that requires grad is being used in an in-place operation: 7. Position / cell update state.positions += dr_atom * unskip frechet_cell_fire in test_optimizer_batch_consistency, can no longer repro error locally * code cleanup * bumpy set-up action to v6, more descriptive CI test names * pin to fairchem_core-1.10.0 in CI * explain differences between vv_fire and ase_fire and link references in fire|unit_cell_fire|frechet_cell_fire doc strings --------- Co-authored-by: Janosh Riebesell <[email protected]>
… ASE vs torch-sim test for Frechet Cell FIRE optimizer into test_optimizers.py - move `ase_mace_mpa` and `torchsim_mace_mpa` fixtures into `conftest.py` for wider reuse
janosh
added
tests
… separate file (thanks @CompRhys)
…nts get redirected in mace-torch
janosh
force-pushed
the
optimizer_ase_test
branch
from
ad950b0 to
03bdb80
Compare
janosh
approved these changes
May 14, 2025
janosh
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
Summary
Adds test to compare ASE vs TorchSim for FIRE optimization.
Currently the trajectories don't match.
Checklist
Work-in-progress pull requests are encouraged, but please enable the draft status on your PR.
Before a pull request can be merged, the following items must be checked:
Run ruff on your code.
Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit installand a check will be run prior to allowing commits.