Add `fix_symmetry: bool = False` option to forcefield relax makers #789

JonathanSchmidt1 · 2024-03-25T14:55:32Z

Summary

Include a summary of major changes in bullet points:

Adds a fix_symmetry option to the Relaxer class for force fields, and the corresponding RelaxMakers that use it (Mace, Nequip, Gap). It's not available for m3gnet and chgnet that use their own internal relaxer. This is very useful when comparing force field relaxations to dft relaxations as the latter usually are fixing the symmetry.
added a new test to force_fields/test_utils.py that confirms that fix_symmetry keeps the symmetry during an example relaxation while the fix_symmetry=False leads to a change in space group.
added fix_symmetry as an option to one of the relaxer test jobs

As far as I can see #722 also does not have any changes conflicting with this one.
Maybe @esoteric-ephemera could confirm this or do you foresee any issues.

Additional dependencies introduced (if any)

None

TODO (if any)

Right now the symprec is fixed to 0.01. One could add an additional symprec parameter or fix_sym_kwargs to allow users to change it. I would suggest adding one of the two options symprec/fix_sym_kwargs but I have no opinion on which one

Checklist

Before a pull request can be merged, the following items must be checked:

Code is in the standard Python style.
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running ruff and ruff format on your new code. This will
automatically reformat your code to PEP8 conventions and fix many linting issues.
Doc strings have been added in the Numpy docstring format.
Run ruff on your code.
Type annotations are highly encouraged. Run mypy to
type check your code.
Tests have been added for any new functionality or bug fixes.
All linting and tests pass.
checked that the force field tests pass

codecov · 2024-03-25T15:13:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.96%. Comparing base (256b39a) to head (a74f008).
Report is 6 commits behind head on main.

❗ Current head a74f008 differs from pull request most recent head 9948246. Consider uploading reports for the commit 9948246 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #789      +/-   ##
==========================================
- Coverage   77.03%   76.96%   -0.07%     
==========================================
  Files         122      122              
  Lines        9108     9127      +19     
  Branches     1429     1430       +1     
==========================================
+ Hits         7016     7025       +9     
- Misses       1663     1675      +12     
+ Partials      429      427       -2

Files	Coverage Δ
src/atomate2/forcefields/jobs.py	`93.52% <100.00%> (+0.61%)`	⬆️
src/atomate2/forcefields/schemas.py	`96.51% <100.00%> (+0.08%)`	⬆️
src/atomate2/forcefields/utils.py	`91.30% <100.00%> (+0.80%)`	⬆️

... and 1 file with indirect coverage changes

esoteric-ephemera · 2024-03-25T16:32:52Z

Thanks @JonathanSchmidt1! This looks fine to me, might just need to add docstr's and such when the MD PR is merged

janosh

thanks @JonathanSchmidt1 just resolved the merge conflicts here now that #722 is merged

tests/forcefields/test_jobs.py

JonathanSchmidt1 · 2024-04-04T14:40:08Z

@janosh Any opinion on this
Right now the symprec is fixed to 0.01. One could add an additional symprec parameter or fix_sym_kwargs to allow users to change it. I would suggest adding one of the two options symprec/fix_sym_kwargs but I have no opinion on which one

janosh · 2024-04-04T14:47:39Z

do we foresee other sym kwargs besides symprec? given pymatgen/spglib has just the 1 parameter, i'm guessing no. so leaning towards adding symprec as a top-level keyword.

esoteric-ephemera · 2024-04-04T16:23:08Z

@janosh: There's also angle_tolerance for spglib

janosh · 2024-04-04T17:52:20Z

@janosh: There's also angle_tolerance for spglib

good point! i think 2 top-level keywords is still okay but i'm easy either way

…/symprec test for MaceRelaxMaker

JonathanSchmidt1 · 2024-04-05T08:38:43Z

It's a good point but as I am using the FixSymmetry Constraint from ase angle tolerance is not an option at the moment. We would have to write our own constraint class or suggest ase to change theirs.
For now I just added a symprec option.
I also noticed that the RelaxMakers were not working with fix symmetry anymore after the merge so I fixed and added additional tests for the chgnet and macerelaxmakers to hopefully make sure it stays working.
I also added fix_symmetry and symprec to the task_doc

JonathanSchmidt1 · 2024-04-05T08:41:46Z

Ps: my latest commit does not seem to show up here let's see if it changes today or if there is some issue

janosh

looks like GH has issues https://www.githubstatus.com:

We are investigating reports of degraded performance.

tests/forcefields/test_jobs.py

…mpatible_results

JonathanSchmidt1 · 2024-04-05T14:55:04Z

sry forgot to run the md tests as well again. Now all tests pass again on my machine. I guess the question is still whether it was correct to change the ForceFieldTaskDocument or if there is some other way to save the fix_symmetry/symprec

esoteric-ephemera · 2024-04-05T15:58:33Z

Regarding the ForceFieldTaskDocument: changes to the schema should be compatible across task doc schemas. Running:

from emmet.core.tasks import TaskDoc
from atomate2.forcefields.schemas import ForceFieldTaskDocument

taskdoc = TaskDoc(**<ForceFieldTaskDocument>.model_dump())
ff_taskdoc = ForceFieldTaskDocument(**taskdoc.model_dump())

should produce no errors. Since both the pydantic models for these docs permit extra top-level fields, I imagine this is OK but good to check

JonathanSchmidt1 · 2024-04-06T11:56:35Z

Regarding the ForceFieldTaskDocument: changes to the schema should be compatible across task doc schemas. Running:
from emmet.core.tasks import TaskDoc
from atomate2.forcefields.schemas import ForceFieldTaskDocument

taskdoc = TaskDoc(**<ForceFieldTaskDocument>.model_dump())
ff_taskdoc = ForceFieldTaskDocument(**taskdoc.model_dump())
should produce no errors. Since both the pydantic models for these docs permit extra top-level fields, I imagine this is OK but good to check

thank you, that worked without issues for a relaxation and an MD task document so I will assume it's fine.

…o 1.5 sec

…r for speed

janosh · 2024-04-07T05:37:53Z

tests/forcefields/test_jobs.py

+    # get space group number of input structure
+    _, final_spg_num = output1.output.structure.get_space_group_info()
+    if fix_symmetry:
+        assert init_spg_num == final_spg_num == 12


@JonathanSchmidt1 this test is failing which seems to suggest fix_symmetry doesn't work for MACE? maybe i'm missing something?

Just checked on my phone so I am sorry if I overlooked sth but it looks to me like the energy checks are failing before right now. Two ideas concerning this:
When Fixing the symmetry the structure is first refined and second you changed the fmax both might lead to a different energy.
If the symmetry check is failing later on which I cannot see on the phone, it might be because we are not determining the initial symmetry with the same symprec in the si-test as the one used for fix_symmetry. I also think the angle tolerance in pymatgen might not be the default one used in ase ( 5 vs -1 I believe).

Gonna take a look later in the evening to see if I can find the problem.

=========================== short test summary info ============================
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-True-True-0.01] - assert -0.07111746072769165 == -0.0526856 ± 5.3e-03

comparison failed
Obtained: -0.07111746072769165
Expected: -0.0526856 ± 5.3e-03
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-True-False-0.01] - assert -0.0711173564195633 == -0.0526856 ± 5.3e-03

comparison failed
Obtained: -0.0711173564195633
Expected: -0.0526856 ± 5.3e-03
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-True-True-0.1] - assert -0.07111746072769165 == -0.0526856 ± 5.3e-03

comparison failed
Obtained: -0.07111746072769165
Expected: -0.0526856 ± 5.3e-03
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-False-True-0.01] - assert -0.06772968173027039 == -0.051912 ± 5.2e-06

comparison failed
Obtained: -0.06772968173027039
Expected: -0.051912 ± 5.2e-06
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-False-False-0.01] - assert -0.06772962212562561 == -0.051912 ± 5.2e-06

comparison failed
Obtained: -0.06772962212562561
Expected: -0.051912 ± 5.2e-06
FAILED tests/forcefields/test_jobs.py::test_mace_relax_makera[/home/runner/work/atomate2/atomate2/tests/test_data/forcefields/mace/MACE.model-False-True-0.1] - assert -0.06772968173027039 == -0.051912 ± 5.2e-06

comparison failed
Obtained: -0.06772968173027039
Expected: -0.051912 ± 5.2e-06

yeah, sorry didn't bother updating the energy in case the changes will be reverted. just commented out the energy assert locally to reach the symmetry check which failed.

pymatgen defaults to angle_tolerance=5 like you said while all the ase call sites for spglib.get_symmetry_dataset i could find pass no explicit angle_tolerance so the default and recommended -1 is used

Symmetry search tolerance in the unit of angle deg. Normally it is not recommended to use this argument. See a bit more detail at angle_tolerance. If the value is negative, an internally optimized routine is used to judge symmetry.

doesn't seem ideal that pymatgen defaults to 5. but would break stuff to change it now...
probably best to just pass si_structure.get_space_group_info(angle_tolerance=-1) here and leave pmg as is

I think the problem might be the following While the space group staying the same means the symmetries are conserved we can of course arrive at a higher symmetry during optimization which changes the space group but is still the correct results for the fix_symmetry setting. Consequently we need to check if the previous symmetry operations are a subset of the new symmetry operations (This is also how the test of Fix_symmetry in ase is done).

from pymatgen.io.ase import AseAtomsAdaptor from ase.spacegroup.symmetrize import check_symmetry, is_subgroup, FixSymmetry si_atoms = AseAtomsAdaptor.get_atoms(si_structure) symmetry_ops_init = check_symmetry(si_atoms, symprec=1.0e-3) si_atoms_final = AseAtomsAdaptor.get_atoms(output1.output.structure) symmetry_ops_final = check_symmetry(si_atoms_final, symprec=1.0e-3) if fix_symmetry: assert is_subgroup(symmetry_ops_init, symmetry_ops_final) else: assert not is_subgroup(symmetry_ops_init, symmetry_ops_final)

I pushed a commit with this but left the rest of the test unchanged so it will still be failing.
If you think it's correct now we can fix the rest.

sorry for letting this slip! that makes sense and thanks for pushing a fix. just updated the test assertions to match our changes

…fixed symmetry

…aterialsproject#789) * add fix_symmetry option and tests to force field relaxer * add fix_symmetry option to one test job * fix linting of test_jobs * add missing documentation to GAP/MACE relaxmaker * restore test_fix_symmetry * restore fix_symmetry to NequipRelaxMaker doc str * add fix_symmetry and symprec back into relaxer jobs. Add fix_symmetry/symprec test for MaceRelaxMaker * add fix_symmetry and symprec to task_doc, add test for chgnet * separated fix_symmetry from standard relax_maker tests * fixed test_chgnet_relax_maker_fix_symmetry * changed fix_symmetry and symprec to optional arguments in from_ase_compatible_results * test_fix_symmetry set steps=1 in relaxer.relax(), takes test from 6 to 1.5 sec * assert expected spacegroup in test_fix_symmetry * cut fix_symmetry and symprec combos in half in test_nequip_relax_maker for speed * parametrize fix_symmetry=True|False on test_mace_relax_maker * test_relax_maker_mace check for subgroup instead of group when using fixed symmetry * fix test_mace_relax_maker --------- Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>

JonathanSchmidt1 added 4 commits March 25, 2024 13:55

add fix_symmetry option and tests to force field relaxer

d7e8109

add fix_symmetry option to one test job

b15944b

fix linting of test_jobs

36d3f11

add missing documentation to GAP/MACE relaxmaker

f5a2bfa

Merge branch 'main' into pr/JonathanSchmidt1/789

7eff631

janosh requested changes Apr 4, 2024

View reviewed changes

tests/forcefields/test_jobs.py Outdated Show resolved Hide resolved

janosh added 2 commits April 4, 2024 11:57

restore test_fix_symmetry

1b40a3a

restore fix_symmetry to NequipRelaxMaker doc str

da7cfd4

JonathanSchmidt1 added 2 commits April 5, 2024 09:33

add fix_symmetry and symprec back into relaxer jobs. Add fix_symmetry…

828e283

…/symprec test for MaceRelaxMaker

add fix_symmetry and symprec to task_doc, add test for chgnet

94a1bdc

janosh reviewed Apr 5, 2024

View reviewed changes

tests/forcefields/test_jobs.py Outdated Show resolved Hide resolved

JonathanSchmidt1 added 3 commits April 5, 2024 15:38

separated fix_symmetry from standard relax_maker tests

73bb548

fixed test_chgnet_relax_maker_fix_symmetry

6f26bfb

changed fix_symmetry and symprec to optional arguments in from_ase_co…

acf08f5

…mpatible_results

JonathanSchmidt1 requested a review from janosh April 6, 2024 12:00

janosh added 4 commits April 6, 2024 14:52

test_fix_symmetry set steps=1 in relaxer.relax(), takes test from 6 t…

c428126

…o 1.5 sec

assert expected spacegroup in test_fix_symmetry

99a5ddf

cut fix_symmetry and symprec combos in half in test_nequip_relax_make…

a74f008

…r for speed

parametrize fix_symmetry=True|False on test_mace_relax_maker

12be77c

janosh reviewed Apr 7, 2024

View reviewed changes

JonathanSchmidt1 and others added 3 commits April 7, 2024 23:26

test_relax_maker_mace check for subgroup instead of group when using …

692d5d3

…fixed symmetry

Merge branch 'main' into fix_symmetry

38523bb

fix test_mace_relax_maker

1caebe0

janosh approved these changes Apr 25, 2024

View reviewed changes

janosh added enhancement Improvements to existing features forcefields Forcefield related symmetry Space groups and the like labels Apr 25, 2024

janosh changed the title ~~Add a fix symmetry option with corresponding test to forcefield relaxations~~ Add fix_symmetry: bool = False option to forcefield relax makers Apr 25, 2024

janosh enabled auto-merge (squash) April 25, 2024 23:52

from ase.(spacegroup.symmetrize->constraints) import FixSymmetry

9948246

janosh merged commit 782abc1 into materialsproject:main Apr 26, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `fix_symmetry: bool = False` option to forcefield relax makers #789

Add `fix_symmetry: bool = False` option to forcefield relax makers #789

JonathanSchmidt1 commented Mar 25, 2024

codecov bot commented Mar 25, 2024 •

edited

Loading

esoteric-ephemera commented Mar 25, 2024

janosh left a comment

JonathanSchmidt1 commented Apr 4, 2024

janosh commented Apr 4, 2024

esoteric-ephemera commented Apr 4, 2024

janosh commented Apr 4, 2024

JonathanSchmidt1 commented Apr 5, 2024

JonathanSchmidt1 commented Apr 5, 2024

janosh left a comment

JonathanSchmidt1 commented Apr 5, 2024

esoteric-ephemera commented Apr 5, 2024

JonathanSchmidt1 commented Apr 6, 2024 •

edited

Loading

janosh Apr 7, 2024

JonathanSchmidt1 Apr 7, 2024

janosh Apr 7, 2024 •

edited

Loading

JonathanSchmidt1 Apr 7, 2024 •

edited by janosh

Loading

JonathanSchmidt1 Apr 7, 2024

janosh Apr 25, 2024

Add fix_symmetry: bool = False option to forcefield relax makers #789

Add fix_symmetry: bool = False option to forcefield relax makers #789

Conversation

JonathanSchmidt1 commented Mar 25, 2024

Summary

Additional dependencies introduced (if any)

TODO (if any)

Checklist

codecov bot commented Mar 25, 2024 • edited Loading

Codecov Report

esoteric-ephemera commented Mar 25, 2024

janosh left a comment

Choose a reason for hiding this comment

JonathanSchmidt1 commented Apr 4, 2024

janosh commented Apr 4, 2024

esoteric-ephemera commented Apr 4, 2024

janosh commented Apr 4, 2024

JonathanSchmidt1 commented Apr 5, 2024

JonathanSchmidt1 commented Apr 5, 2024

janosh left a comment

Choose a reason for hiding this comment

JonathanSchmidt1 commented Apr 5, 2024

esoteric-ephemera commented Apr 5, 2024

JonathanSchmidt1 commented Apr 6, 2024 • edited Loading

janosh Apr 7, 2024

Choose a reason for hiding this comment

JonathanSchmidt1 Apr 7, 2024

Choose a reason for hiding this comment

janosh Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

JonathanSchmidt1 Apr 7, 2024 • edited by janosh Loading

Choose a reason for hiding this comment

JonathanSchmidt1 Apr 7, 2024

Choose a reason for hiding this comment

janosh Apr 25, 2024

Choose a reason for hiding this comment

Add `fix_symmetry: bool = False` option to forcefield relax makers #789

Add `fix_symmetry: bool = False` option to forcefield relax makers #789

codecov bot commented Mar 25, 2024 •

edited

Loading

JonathanSchmidt1 commented Apr 6, 2024 •

edited

Loading

janosh Apr 7, 2024 •

edited

Loading

JonathanSchmidt1 Apr 7, 2024 •

edited by janosh

Loading