Implementation to adjust get_supercell_size to also generate orthorhombic supercells

[QuantumChemist]

Summary

I'm adjusting get_supercell_size to also generate orthorhombic supercells. This implementation is depending on https://github.com/materialsproject/pymatgen/pull/3938

[QuantumChemist]

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]

The failing AIMS unit tests have been addressed here https://github.com/materialsproject/atomate2/pull/928

[QuantumChemist]

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]

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

[QuantumChemist]

I also saw that the pymatgen version gets updated anyway :) https://github.com/materialsproject/atomate2/pull/953

[QuantumChemist]

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

[JaGeo]

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

[QuantumChemist]

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

thank you! I'm going to fix them over the next few days as it's quite a few that fail.

[QuantumChemist]

So... openff-toolkit is missing in the pyproject.toml, but actually, I have problems of getting it installed at all. I encounter the problem: ERROR: Could not find a version that satisfies the requirement openff-toolkit (from atomate2) (from versions: none) ERROR: No matching distribution found for openff-toolkit Seems there is no version. I'm using Python 3.10. Shall I switch to another Python version?

[utf]

@janosh @orionarcher are you able to advise. Presumably this should not need to be installed to use the rest of the code?

[janosh]

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)

[orionarcher]

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]

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]

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]

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]

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]

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

[janosh]

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

[QuantumChemist]

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]

@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]

@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!

[QuantumChemist]

@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 😄

[QuantumChemist]

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]

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]

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

[QuantumChemist]

@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]

matgl is brittle as hell. had to skip 2 tests in https://github.com/materialsproject/atomate2/pull/990. once that merges, CI should be stable

[QuantumChemist]

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]

Thank you for merging this! :)