Implementation to adjust get_supercell_size to also generate orthorhombic supercells #923

QuantumChemist · 2024-07-19T17:45:35Z

Summary

I'm adjusting get_supercell_size to also generate orthorhombic supercells.
This implementation is depending on materialsproject/pymatgen#3938

QuantumChemist · 2024-07-26T18:47:49Z

I don't exactly understand how I caused the TypeError: get_supercell_size() missing 1 required positional argument: 'prefer_90_degrees' and the other problems with the phonon unit tests with my changes, yet 😅

QuantumChemist · 2024-07-30T15:39:08Z

pyproject.toml

@@ -101,7 +101,7 @@ strict = [
    "pydantic-settings==2.3.4",
    "pydantic==2.8.2",
    "pymatgen-analysis-defects==2024.7.19",
-    "pymatgen==2024.6.10",
+    "pymatgen@git+https://github.com/materialsproject/pymatgen.git@8e392947348c75220cfb79309fe8fc9745edc580",  # has to be adjusted with new pymatgen release


I have fixed the pymatgen version to the merge that contains the changes in CubicSupercellTransformation in pymatgen.

Thanks @QuantumChemist but we can't merge until a new pymatgen version has been released and this has been updated.

Hi @utf , Ok when the new pymatgen release is out, I will also adjust it here :)

QuantumChemist · 2024-07-30T17:04:11Z

The failing AIMS unit tests have been addressed here #928

QuantumChemist · 2024-07-30T17:25:00Z

Hey @utf ! 😃

in principle this PR is ready to be reviewed and merged. The failing tests all stem from AIMS:

tests/abinit/flows/test_core.py ...                                      [  0%]
tests/abinit/jobs/test_core.py ....                                      [  2%]
tests/abinit/sets/test_base.py ...........                               [  5%]
tests/abinit/sets/test_core.py ...                                       [  6%]
tests/abinit/test_powerups.py ....                                       [  7%]
tests/aims/test_files/test_files.py .                                    [  7%]
tests/aims/test_flows/test_core.py F                                     [  8%]
tests/aims/test_flows/test_elastic.py FF                                 [  8%]
tests/aims/test_flows/test_eos.py FF                                     [  9%]
tests/aims/test_flows/test_gw_convergence.py F                           [  9%]
tests/aims/test_flows/test_phonon_workflow.py FsFs                       [ 11%]
tests/aims/test_makers/test_convergence.py F                             [ 11%]
tests/aims/test_makers/test_gw.py F                                      [ 11%]
tests/aims/test_makers/test_relax.py FF                                  [ 12%]
tests/aims/test_makers/test_socket_calc.py s                             [ 12%]
tests/aims/test_makers/test_static.py F                                  [ 12%]
tests/common/jobs/test_elastic.py ...........................            [ 21%]
tests/common/jobs/test_eos.py ...                                        [ 22%]
tests/common/jobs/test_phonon.py ...                                     [ 22%]
tests/common/schemas/test_cclib.py ..                                    [ 23%]
tests/common/schemas/test_defect.py ...                                  [ 24%]
tests/common/schemas/test_elastic.py .....                               [ 25%]
tests/common/schemas/test_phonons.py ....                                [ 27%]
tests/common/test_files.py ..                                            [ 27%]
tests/common/test_jobs.py ..s                                            [ 28%]
tests/common/test_settings.py .                                          [ 29%]
tests/cp2k/jobs/test_core.py ...                                         [ 29%]
tests/cp2k/sets/test_base.py ..                                          [ 30%]
tests/cp2k/sets/test_core.py .                                           [ 30%]
tests/cp2k/test_drones.py .                                              [ 31%]
tests/cp2k/test_files.py ..                                              [ 31%]
tests/cp2k/test_powerups.py .....                                        [ 33%]
tests/forcefields/flows/test_elastic.py .                                [ 33%]
tests/forcefields/flows/test_eos.py ...                                  [ 34%]
tests/forcefields/flows/test_phonon.py .                                 [ 34%]
tests/forcefields/test_jobs.py ...........................               [ 43%]
tests/forcefields/test_md.py ...........                                 [ 46%]
tests/forcefields/test_utils.py ..........                               [ 49%]
tests/qchem/jobs/test_core.py ...                                        [ 50%]
tests/qchem/sets/test_core.py ..............................             [ 59%]
tests/qchem/test_drones.py ..                                            [ 60%]
tests/qchem/test_files.py ...                                            [ 61%]
tests/qchem/test_run.py ..                                               [ 61%]
tests/qchem/test_sets.py ......                                          [ 63%]
tests/vasp/flows/test_core.py .........                                  [ 66%]
tests/vasp/flows/test_defect.py ....                                     [ 67%]
tests/vasp/flows/test_elastic.py ...                                     [ 68%]
tests/vasp/flows/test_electrode.py .                                     [ 68%]
tests/vasp/flows/test_elph.py .                                          [ 69%]
tests/vasp/flows/test_eos.py ...                                         [ 70%]
tests/vasp/flows/test_magnetism.py .                                     [ 70%]
tests/vasp/flows/test_matpes.py .                                        [ 70%]
tests/vasp/flows/test_md.py ..                                           [ 71%]
tests/vasp/flows/test_mp.py ...........                                  [ 74%]
tests/vasp/flows/test_phonons.py .................                       [ 79%]
tests/vasp/jobs/test_core.py ......                                      [ 81%]
tests/vasp/jobs/test_eos.py ..                                           [ 82%]
tests/vasp/jobs/test_matpes.py ....                                      [ 83%]
tests/vasp/jobs/test_md.py .                                             [ 83%]
tests/vasp/jobs/test_mp.py .....                                         [ 85%]
tests/vasp/lobster/flows/test_lobster.py ....                            [ 86%]
tests/vasp/lobster/schemas/test_lobster.py ...                           [ 87%]
tests/vasp/schemas/test_defect.py .                                      [ 87%]
tests/vasp/sets/test_matpes.py ..                                        [ 88%]
tests/vasp/sets/test_mp.py ....                                          [ 89%]
tests/vasp/test_drones.py .                                              [ 89%]
tests/vasp/test_files.py .......                                         [ 92%]
tests/vasp/test_powerups.py ........                                     [ 94%]
tests/vasp/test_run.py .                                                 [ 94%]
tests/vasp/test_sets.py .................                                [100%]

Thank you a lot!

QuantumChemist · 2024-08-09T14:03:44Z

Hey @utf , I updated the pymatgen version as they released a new version. The failing unit tests still stem from aims. Thank you 😃

QuantumChemist · 2024-08-12T09:23:07Z

I also saw that the pymatgen version gets updated anyway :) #953

…mChemist/atomate2 into adjust_get_supercell_size

QuantumChemist · 2024-08-23T10:42:01Z

Hey @utf , I just wanted to ask you if my PR is now ok to be merged? Thank you 😃

JaGeo · 2024-08-23T20:37:19Z

src/atomate2/common/jobs/phonons.py


    if not prefer_90_degrees:
        transformation = CubicSupercellTransformation(
-            **common_kwds, max_atoms=kwargs.get("max_atoms"), force_90_degrees=False
+            **common_kwds,
+            max_atoms=kwargs.get("max_atoms"),


why is this not part of theother dict?

It was like this in the original implementation, so I left it like that.

If you would like me to, I could move it to common_kwds as I don't think it will cause any backwards incompatibility issues and I would find it more systematic to have all kwargs in common_kwds.

Sounds good

I forgot to push those changes, I notice now, but it doesn't affect the functionalities.

I finally thought of pushing this change 😅

…/atomate2 into adjust_get_supercell_size

JaGeo · 2024-09-24T14:30:02Z

@QuantumChemist Thanks! I am happy to merge if the tests pass 😀

orionarcher · 2024-09-26T13:55:50Z

As @janosh said, openff-toolkit isn't on PyPI which would explain the original error. openff.toolkit shouldn't need to be installed to use any code outside the openff module and openmm.generate (though certainly possible I made a mistake). A little more context for the error would be helpful!

QuantumChemist · 2024-09-26T14:05:58Z

As @janosh said, openff-toolkit isn't on PyPI which would explain the original error. openff.toolkit shouldn't need to be installed to use any code outside the openff module and openmm.generate (though certainly possible I made a mistake). A little more context for the error would be helpful!

oh, I see. The issue came up because I tried pytest test/* to see which unit tests are currently failing. Then I will simply exclude the openff tests. Thank you!

QuantumChemist · 2024-09-26T14:06:54Z

Presumably this should not need to be installed to use the rest of the code?

yes, i think the intent is to only raise an ImportError for openff.toolkit if a user tries to create any of the OpenMM makers. @QuantumChemist can you post the full stack trace?

the reason it's not in pyproject.toml is because the package is not on PyPI (only installable with conda/mamba)

installing with conda somehow messed up my conda env. I will simply exclude thes openff tests. Thank you 😃

QuantumChemist · 2024-09-26T14:11:12Z

Presumably this should not need to be installed to use the rest of the code?

yes, i think the intent is to only raise an ImportError for openff.toolkit if a user tries to create any of the OpenMM makers. @QuantumChemist can you post the full stack trace?

the reason it's not in pyproject.toml is because the package is not on PyPI (only installable with conda/mamba)

ah and I forgot to post the whole error message:

ImportError while loading conftest '/home/certural/git/chris/atomate2/tests/openff_md/conftest.py'.
tests/openff_md/conftest.py:2: in <module>
    import openff.toolkit as tk
E   ModuleNotFoundError: No module named 'openff'

so this happens when running all unit tests at once.

janosh · 2024-09-26T14:32:48Z

it's probably best to pass the file/directory path to only the tests you currently care about to pytest and leave running the complete test suite to CI. that said, @orionarcher will submit a PR to sprinkle in some pytest.importorskip("openmm") in the OpenMM test modules to avoid future issues.

a bunch of the FF tests also fail unless you have all the FF packages installed. so the test suite is not currently designed to be out-of-the-box runnable by contributors who only care about part of the code and don't have all optional deps installed

JaGeo · 2024-09-26T14:37:21Z

@janosh is the issue with the defects part also still existing? I usually had to install the defects add-on for the tests.

janosh · 2024-09-26T14:41:09Z

i'm not sure, haven't run the complete test suite locally in forever 😅

QuantumChemist · 2024-09-26T14:41:52Z

it's probably best to pass the file/directory path to only the tests you currently care about to pytest and leave running the complete test suite to CI. that said, @orionarcher will submit a PR to sprinkle in some pytest.importorskip("openmm") in the OpenMM test modules to avoid future issues.

a bunch of the FF tests also fail unless you have all the FF packages installed. so the test suite is not currently designed to be out-of-the-box runnable by contributors who only care about part of the code and don't have all optional deps installed

I usually do but I also run an extra pytest test/* because, I don't know in what submodules the code I changed is called. As you said, I will then let that be handled by the CI :)

QuantumChemist · 2024-09-26T14:51:56Z

@utf could you please resolve the merge conflicts? I can't do it, it seems. The relevant unit tests are passing locally for me.

orionarcher · 2024-09-26T15:39:06Z

@QuantumChemist to resolve the merge conflict you can:

update your local main branch
merge main into your local adjust_get_supercell_size
push the merge commit to the remote adjust_get_supercell_size (this branch)

That should do it!

QuantumChemist · 2024-09-26T15:41:11Z

@QuantumChemist to resolve the merge conflict you can:

1. update your local main branch

2. merge main into your local adjust_get_supercell_size

3. push the merge commit to the remote adjust_get_supercell_size (this branch)

That should do it!

Right, I'm so used to do this on the GitHub website that I didn't think of this possibility! Thank you 😄

…size

QuantumChemist · 2024-09-26T16:50:53Z

Regarding the failing unit tests:
cp2k fails because self.data.get("total_energy") yields three energies [-193.39152045030943, -193.39152037984607, -193.39152037984607], from which the first one corresponds to the last total energy entry in the given test file.

For the failing aims and lobster tests I have no idea 😅

QuantumChemist · 2024-09-30T10:12:59Z

I don't understand what is going on with the unit tests... I will try with a fresh MP/atomate2/main branch and open another PR.

JaGeo · 2024-09-30T10:14:41Z

@QuantumChemist there are a few problems currently. @janosh was working on some of them already.

QuantumChemist · 2024-09-30T10:20:05Z

@QuantumChemist there are a few problems currently. @janosh was working on some of them already.

ah, I see. I was just wondering why the unit tests don't fail for the MP repo or other PRs. I thought I caused it. Then I will just wait :) Thank you!

janosh · 2024-09-30T10:23:32Z

matgl is brittle as hell. had to skip 2 tests in #990. once that merges, CI should be stable

QuantumChemist · 2024-09-30T11:07:16Z

matgl is brittle as hell. had to skip 2 tests in #990. once that merges, CI should be stable

Ok, great! :D Thank you a lot!

QuantumChemist · 2024-10-03T21:40:53Z

Thank you for merging this! :)

…mbic supercells (materialsproject#923) * starting with implementation to adjust get_supercell_size * adding orthorhombic supercells * add unit test * fixed the reason for the failing unit tests * adjust pymatgen version * adjust pymatgen version * fix linting * fix missing max_length in unit test * adjusted pymatgen version * add step_size to common_kwds * put max_atoms to common_kwds * adjustments to make unit tests pass * fixing ase unit test * Update test_jobs.py --------- Co-authored-by: J. George <[email protected]>

QuantumChemist added 3 commits July 19, 2024 19:42

starting with implementation to adjust get_supercell_size

3deb9d2

adding orthorhombic supercells

a528d79

add unit test

548c134

QuantumChemist marked this pull request as ready for review July 26, 2024 18:16

Merge branch 'main' into adjust_get_supercell_size

5612c2c

QuantumChemist changed the title ~~[WIP] Implementation to adjust get_supercell_size to also generate orthorhombic supercells~~ Implementation to adjust get_supercell_size to also generate orthorhombic supercells Jul 26, 2024

QuantumChemist and others added 5 commits July 30, 2024 16:09

fixed the reason for the failing unit tests

4aee0fb

Merge branch 'main' into adjust_get_supercell_size

2932df2

adjust pymatgen version

de887d2

adjust pymatgen version

21161de

fix linting

25968f4

QuantumChemist commented Jul 30, 2024

View reviewed changes

fix missing max_length in unit test

c979b1a

QuantumChemist and others added 2 commits August 9, 2024 14:26

adjusted pymatgen version

f7b3741

Merge branch 'main' into adjust_get_supercell_size

5903241

QuantumChemist and others added 3 commits August 13, 2024 14:58

add step_size to common_kwds

d5567b8

Merge branch 'adjust_get_supercell_size' of https://github.com/Quantu…

e79f32f

…mChemist/atomate2 into adjust_get_supercell_size

Merge branch 'materialsproject:main' into adjust_get_supercell_size

5afb212

JaGeo reviewed Aug 23, 2024

View reviewed changes

JaGeo and others added 4 commits September 7, 2024 18:37

Merge branch 'main' into adjust_get_supercell_size

b8392fe

put max_atoms to common_kwds

12198dd

Merge branch 'adjust_get_supercell_size' of github.com:QuantumChemist…

aa38a22

…/atomate2 into adjust_get_supercell_size

Merge branch 'materialsproject:main' into adjust_get_supercell_size

85274ec

QuantumChemist added 2 commits September 26, 2024 17:45

Merge remote-tracking branch 'origin/main' into adjust_get_supercell_…

ccd392c

…size

fixing ase unit test

9fbeabe

orionarcher mentioned this pull request Sep 26, 2024

Excise openff dependency from OpenMM testing #993

Merged

Merge branch 'materialsproject:main' into adjust_get_supercell_size

ed46087

Merge branch 'main' into adjust_get_supercell_size

5320236

JaGeo mentioned this pull request Oct 3, 2024

Bump lobsterpy from 0.4.5 to 0.4.9 #1001

Merged

JaGeo added 2 commits October 3, 2024 19:49

Merge branch 'main' into adjust_get_supercell_size

0c2a9ac

Update test_jobs.py

3397a2b

JaGeo enabled auto-merge (squash) October 3, 2024 17:55

JaGeo merged commit 4944aed into materialsproject:main Oct 3, 2024
15 checks passed

utf added the enhancement Improvements to existing features label Nov 12, 2024

Implementation to adjust get_supercell_size to also generate orthorhombic supercells #923

Implementation to adjust get_supercell_size to also generate orthorhombic supercells #923

Conversation

QuantumChemist commented Jul 19, 2024

Summary

QuantumChemist commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuantumChemist commented Jul 30, 2024

QuantumChemist commented Jul 30, 2024

QuantumChemist commented Aug 9, 2024

QuantumChemist commented Aug 12, 2024

QuantumChemist commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaGeo commented Sep 24, 2024

orionarcher commented Sep 26, 2024 • edited Loading

QuantumChemist commented Sep 26, 2024

QuantumChemist commented Sep 26, 2024

QuantumChemist commented Sep 26, 2024

janosh commented Sep 26, 2024

JaGeo commented Sep 26, 2024

janosh commented Sep 26, 2024

QuantumChemist commented Sep 26, 2024 • edited Loading

QuantumChemist commented Sep 26, 2024

orionarcher commented Sep 26, 2024 • edited Loading

QuantumChemist commented Sep 26, 2024 • edited Loading

QuantumChemist commented Sep 26, 2024

QuantumChemist commented Sep 30, 2024

JaGeo commented Sep 30, 2024

QuantumChemist commented Sep 30, 2024 • edited Loading

janosh commented Sep 30, 2024

QuantumChemist commented Sep 30, 2024

QuantumChemist commented Oct 3, 2024

orionarcher commented Sep 26, 2024 •

edited

Loading

QuantumChemist commented Sep 26, 2024 •

edited

Loading

orionarcher commented Sep 26, 2024 •

edited

Loading

QuantumChemist commented Sep 26, 2024 •

edited

Loading

QuantumChemist commented Sep 30, 2024 •

edited

Loading