Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Max force handler and r2scan fixes #778

Merged

Conversation

MichaelWolloch
Copy link
Contributor

Summary

This fixes 2 bugs that were recently mentioned in issues #776 and #774

  • Removes all mentions of the deprecated and (since version 2023.5.7 removed) MaxForceErrorHandler for VASP. Also removed associated max_force_threshold (which was used as an input to MaxForceErrorHandler from OptimizeFW and RunVaspCustodian. Similarly, RELAX_MAX_FORCE, which was the default for max_force_threshold was removed from atomate.vasp.config.py and atomate.vasp.vasp_config.py
  • Changes the BPARAM in ScanOptimizeFW from 15.7 to 11.95. This is the right parameter to select R2SCAN+rVV10, while the old one was optimized for SCAN+rVV10, which is no longer used in ScanOptimizeFW.

Since the first bug does break VASP functionality, at least if RunVaspCustodian is used, and some of the latest custodian releases are used, which does happen for a fresh install since no upper version limit is set, I think this is a quite important fix. Probably a new release should be made as well.

@MichaelWolloch
Copy link
Contributor Author

Had forgotten to change the BPARAM also in the reference INCAR file for TestScanOptimizeWorkflow. Done now. I am not sure if the other failing tests have something to do with my changes, but doubt it. Unfortunately I have troubles testing this on my machine due to database issues.

@MichaelWolloch
Copy link
Contributor Author

OK, after downgrading my mongodb installation to 5.x and installing openbabel, I got the tests to run locally. All pass or are skipped, so I am not sure why the testing environment fails.

================================================ test session starts =================================================
platform linux -- Python 3.11.0, pytest-7.2.2, pluggy-1.0.0
rootdir: /fs/home/wolloch/git_test/mwo_atomate/atomate, configfile: setup.cfg
plugins: anyio-3.6.2
collecting 0 items                                                                                                   /fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/common/firetasks/tests/test_parse_outputs.py:17: PytestCollectionWarning: cannot collect test class 'TestDrone' because it has a __init__ constructor (from: atomate/common/firetasks/tests/test_parse_outputs.py)
  class TestDrone(AbstractDrone):
/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/utils/tests/test_database.py:22: PytestCollectionWarning: cannot collect test class 'TestToDb' because it has a __init__ constructor (from: atomate/utils/tests/test_database.py)
  class TestToDb(CalcDb):
collected 226 items                                                                                                  

atomate/common/firetasks/tests/test_glue_tasks.py .......                                                      [  3%]
atomate/common/firetasks/tests/test_parse_outputs.py .                                                         [  3%]
atomate/common/tests/test_powerups.py ......                                                                   [  6%]
atomate/feff/firetasks/tests/test_tasks.py ..                                                                  [  7%]
atomate/feff/fireworks/tests/test_fireworks.py .                                                               [  7%]
atomate/feff/workflows/tests/test_eels_workflows.py ...                                                        [  8%]
atomate/feff/workflows/tests/test_exafs_scattering_paths.py ...                                                [ 10%]
atomate/feff/workflows/tests/test_xas_workflows.py ...                                                         [ 11%]
atomate/lammps/tests/test_drone.py s                                                                           [ 11%]
atomate/lammps/tests/test_workflows.py s                                                                       [ 12%]
atomate/qchem/firetasks/tests/test_critic2.py s.                                                               [ 13%]
atomate/qchem/firetasks/tests/test_fragmenter.py ................                                              [ 20%]
atomate/qchem/firetasks/tests/test_geo_transformations.py ..                                                   [ 21%]
atomate/qchem/firetasks/tests/test_parse_outputs.py ....                                                       [ 23%]
atomate/qchem/firetasks/tests/test_run_calc.py ..........                                                      [ 27%]
atomate/qchem/firetasks/tests/test_write_inputs.py .........                                                   [ 31%]
atomate/qchem/fireworks/tests/test_core.py ...................                                                 [ 39%]
atomate/qchem/tests/test_drones.py ...................                                                         [ 48%]
atomate/qchem/tests/test_powerups.py .                                                                         [ 48%]
atomate/qchem/workflows/tests/test_FF_and_critic.py .                                                          [ 49%]
atomate/qchem/workflows/tests/test_double_FF_opt.py .                                                          [ 49%]
atomate/qchem/workflows/tests/test_fragmentation.py ..                                                         [ 50%]
atomate/qchem/workflows/tests/test_parse_pass_write.py ..                                                      [ 51%]
atomate/qchem/workflows/tests/test_reaction_path.py .                                                          [ 51%]
atomate/qchem/workflows/tests/test_torsion_potential.py .                                                      [ 52%]
atomate/utils/tests/test_database.py ....                                                                      [ 53%]
atomate/utils/tests/test_loaders.py ..                                                                         [ 54%]
atomate/utils/tests/test_utils.py ......                                                                       [ 57%]
atomate/vasp/firetasks/tests/test_copy.py ......                                                               [ 60%]
atomate/vasp/firetasks/tests/test_exchange.py s                                                                [ 60%]
atomate/vasp/firetasks/tests/test_get_interpolated_poscar.py .                                                 [ 61%]
atomate/vasp/firetasks/tests/test_lobster_tasks.py .......                                                     [ 64%]
atomate/vasp/firetasks/tests/test_polarization_to_db.py .                                                      [ 64%]
atomate/vasp/firetasks/tests/test_write_vasp.py ........                                                       [ 68%]
atomate/vasp/firetasks/tests/test_write_vasp_from_interpolated_poscar.py .                                     [ 68%]
atomate/vasp/fireworks/tests/test_exchange.py .                                                                [ 69%]
atomate/vasp/fireworks/tests/test_fireworks_core.py .........                                                  [ 73%]
atomate/vasp/fireworks/tests/test_lobster.py .                                                                 [ 73%]
atomate/vasp/fireworks/tests/test_nmr.py .                                                                     [ 73%]
atomate/vasp/tests/test_drones.py ........                                                                     [ 77%]
atomate/vasp/tests/test_setup.py .                                                                             [ 77%]
atomate/vasp/tests/test_vasp_powerups.py ..............                                                        [ 84%]
atomate/vasp/workflows/tests/test_adsorbate_workflow.py ..s                                                    [ 85%]
atomate/vasp/workflows/tests/test_approx_neb_workflow.py .                                                     [ 85%]
atomate/vasp/workflows/tests/test_bulk_modulus_workflow.py .                                                   [ 86%]
atomate/vasp/workflows/tests/test_elastic_workflow.py .                                                        [ 86%]
atomate/vasp/workflows/tests/test_exchange_workflow.py s                                                       [ 87%]
atomate/vasp/workflows/tests/test_ferroelectric_workflow.py .                                                  [ 87%]
atomate/vasp/workflows/tests/test_hubbard_hund_linresp.py .                                                    [ 88%]
atomate/vasp/workflows/tests/test_insertion_workflow.py .                                                      [ 88%]
atomate/vasp/workflows/tests/test_lobster_workflow.py ......                                                   [ 91%]
atomate/vasp/workflows/tests/test_magnetism_workflow.py .s.                                                    [ 92%]
atomate/vasp/workflows/tests/test_neb_workflow.py .                                                            [ 92%]
atomate/vasp/workflows/tests/test_nmr.py .                                                                     [ 93%]
atomate/vasp/workflows/tests/test_raman_workflow.py .                                                          [ 93%]
atomate/vasp/workflows/tests/test_vasp_workflows.py ..............                                             [100%]

===================================== 219 passed, 7 skipped in 351.20s (0:05:51) =====================================

@MichaelWolloch
Copy link
Contributor Author

The exact same test also fail for PR #775, so I am really confident now that something is wrong with the CI, rather than my code. I have really no experience with CI, but the versions of python, pytest, and pluggy are significantly older than on my local machine. Maybe one should upgrade those?

@janosh
Copy link
Member

janosh commented Jul 28, 2023

Feel free to update the req files. No reason to pin to an old pymatgen.

pymatgen==2023.3.10

@MichaelWolloch
Copy link
Contributor Author

Hi @janosh , and thanks for pointing that out. I did not really notice the requirements file to be honest.

The tests that I claimed passed on my machine were actually using the old pymatgen and custodian versions. Once I updated, I also got the same two failed tests, test_babel_PC_with_RO_depth_0_vs_depth_10 in atomate/qchem/firetasks/tests/test_fragmenter.py and TestNMRWorkflow in atomate/vasp/workflows/tests/test_nmr.py.

I wanted to at least figure out why the vasp related test is failing, and has been failing since may. It turned out that figuring that out was quite a challenge, that's why I am only responding now.

It was pretty clear that the vasprun.xml file could not be loaded into a pymatgen Vasprun object, but I could not make much sense of the traceback and no relevant code in the Vasprun class was changed as far as I could see. Once I figured out that indeed nothing at all changed in the Vasprun class between the last working version (v2023.3.28) and the next release (v2023.5.8) I went through changes in the imports and, by some trial and error (I should really learn how to debug python modules better) figured out that the breaking change was actually something I (and @janosh ) are a bit familiar with: Commit materialsproject/pymatgen@1e9119a !

Apparently the change in the Potcar class did break the reading of vasprun.xml files for chemical shift tensor calculation (LCHIMAG = .TRUE.). I am not sure exactly how, but I spent enough time on this already, so I am content with the knowledge that it did.

I will make a comment in the appropriate issue (materialsproject/pymatgen#3068) and also make a PR for this change I guess. However, for now I will update the requirements to the current versions and let this test fail.

I will also take a look at the other failed test, but since I know nothing about qchem, this will probably not result in much success. However, that test fails pretty straightforwardly with AssertionError: 411 != 509, so maybe I can figure out what happens after all.

@janosh
Copy link
Member

janosh commented Aug 1, 2023

@MichaelWolloch Thanks for taking a deep dive on this! It's very helpful to know why the test is failing. I just took a look in pymatgen. All the vasprun.xml files we have set LCHIMAG=False which might explain why we didn't catch this. I'm not at all familiar with chemical shift tensor calculation. It's not clear to me though why a change in POTCAR parsing would affect parsing vasprun.xml. Any ideas @jmmshn? In any case, maybe we can take the vasprun.xml in the failing test and use it for a new test in pymatgen so we don't regress again.

Screenshot 2023-08-01 at 6 56 19 AM

@MichaelWolloch
Copy link
Contributor Author

I think it somehow has to do with setting charges from POTCAR. At least that what the traceback suggests:

Traceback (most recent call last):
  File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 270, in generate_doc
    d["calcs_reversed"] = [
                          ^
  File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 271, in <listcomp>
    self.process_vasprun(dir_name, taskname, filename)
  File "/fs/home/wolloch/git_test/mwo_atomate/atomate/atomate/vasp/drones.py", line 451, in process_vasprun
    vrun = Vasprun(vasprun_file, parse_potcar_file=self.parse_potcar_file)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 395, in __init__
    self.update_charge_from_potcar(parse_potcar_file)
  File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 1152, in update_charge_from_potcar
    for s in self.structures:
             ^^^^^^^^^^^^^^^
  File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 518, in structures
    return [step["structure"] for step in self.ionic_steps]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/michael/mambaforge/envs/atomate_test/lib/python3.11/site-packages/pymatgen/io/vasp/outputs.py", line 518, in <listcomp>
    return [step["structure"] for step in self.ionic_steps]
            ~~~~^^^^^^^^^^^^^
KeyError: 'structure'

I agree with making a new test using a LCHIMAG=True vasprun.xml.
Actually I also never made a chemical shift tensor calculation before yesterday. I wanted to re-do the calculation with vasp 6.4.1 to see if there was maybe a problem with the old 5.4.4 version used in the atomate test. It was not.

@MichaelWolloch
Copy link
Contributor Author

MichaelWolloch commented Aug 1, 2023

Regarding the CI. Apparently the changes to the requirements.txt and requirements-ci.txt made things considerably worse :/ now. The tests cannot even be collected it seems. I think this might be an issue with openbabel, which I needed to install via mamba/conda locally because the pip install fails with a broken wheel. But I think I fixed it by using openbabel-wheel in requirements-ci.txt instead of not specifying openbabel at all.

@MichaelWolloch
Copy link
Contributor Author

OK, that did not quite do the trick...
@janosh , can you help me with how the CI is set up? I assumed it would install both requirements.txt and requirements-ci.txt. Doing this in that order worked for me in a fresh environment. Or is only pip install -r requirements-ci.txt used in the CI?

@janosh
Copy link
Member

janosh commented Aug 1, 2023

Looks like requirements.txt is unused:

pip install -r requirements-ci.txt
pip install .[complete]

So you need to modify setup.py

atomate/setup.py

Lines 25 to 43 in f4060e5

install_requires=[
"custodian>=2019.8.24",
"FireWorks>=2.0.3",
"maggma>=0.44.0",
"monty>=2.0.6",
"networkx",
"numpy",
"pandas",
"paramiko",
"pydash>=4.1.0",
"pymatgen-analysis-diffusion",
"pymatgen-analysis-defects>=2022.9.14",
"pymatgen",
"pymongo",
"pyyaml>=5.1.2",
"ruamel.yaml",
"scipy",
"tqdm>=4.7.4",
],

@MichaelWolloch
Copy link
Contributor Author

Thanks for the info @janosh, I will try to do update the setup file instead!

I also found out a bit more about the Vaspruns with LCHIMAG. It is even more of an edge case than I thought.

Actually there was already a test for that in pymatgen, and there is a corresponding vasprun.xml file! The file is just called vasprun.xml.chemical_shift.scstep so it did not show up in your search.

Here is the test in pymatgen.io.vasp.tests.test_outputs.py VasprunTest class:

    def test_parsing_chemical_shift_calculations(self):
        with warnings.catch_warnings():
            warnings.simplefilter("ignore")
            filepath = self.TEST_FILES_DIR / "nmr" / "cs" / "basic" / "vasprun.xml.chemical_shift.scstep"
            vasprun = Vasprun(filepath)
            nestep = len(vasprun.ionic_steps[-1]["electronic_steps"])
            assert nestep == 10
            assert vasprun.converged

And this test passes with the current pymatgen release!

However, in that folder, there is no POTCAR file. So when the vasprun.xml file is loaded, the update_charge_from_potcar method of the Vasprun class is not used, and the problematic parsing of the POTCAR does not occur.

So the test correctly parses the vasprun file, but if a POTCAR is in the same folder, as will be the case usually for a real calculation, and as is the case for the atomate workflow test, the parsing fails.

No wonder that this eluded everyone for so long.

@jmmshn
Copy link
Contributor

jmmshn commented Aug 1, 2023

@MichaelWolloch thanks for catching this. So there was a bug with the POTCAR output from pymatgen while we were fixing a couple of POTCAR issues.
Have you confirmed that the POTCARs in those directories were generated using the new pymatgen release as well?

Please confirm and pay special attention to the end of each atomic data block.

@MichaelWolloch
Copy link
Contributor Author

Ok, I now updated setup.py to use more modern versions.

I decided to include openbabel-wheel into the extra-requires for qchem and complete, because openbabel does not work with pip. In the CI it is instead installed via apt-get, but I think one should be bale to run tests when just installing everything with pip. In fact, I would actually remove requirements.txt and requirements-ci.txt altogether and put everything in setup.py if it were my call. Are there any objections to this?

@MichaelWolloch
Copy link
Contributor Author

@jmmshn, regarding the POTCARs, this is a very good point. The one I tested was just taken from atomate/vasp/test_files/nmr_wf/cs/outputs. So definitely an old one.
I now did create a new POTCAR using MPNMRSet and the latest pymatgen release. The error stays the same.

However, I now tested a lot more stuff in pymatgen and found that the way it was done in pymatgen version 2023.3.28 was actually wrong as well. The splitting was not done quite right, leading to a problem accessing the headers of the PotcarSingles. (this is of course what @janosh and @jmmshn fixed in their commit materialsproject/pymatgen@1e9119a ! Although they introduced some other issues at that time.) This leads to an error after potcar parsing that prevents the charges from being updated in the structures of the Vasprun. If that error is fixed, the loading of the Vasprun fails again.

The curx of the problem seems to be the fact that even if NSW=0 for a chemical shift calculation, there are more than 1 ionic steps counted by the vasprun parser. Usually each of those steps has a structure, and then the Vasprun.update_charge_from_potcar method tries to update the charges for each of those. For the chemical shift calculations, only the first "ionic step" has actually a structure attached. Thus I did an additional check for a chemical shift calculation.

I think further discussion on that topic should be done on the pymatgen github. Maybe in materialsproject/pymatgen#3068, or in a new PR that I will make there today.

I did add an additional test case for a chemical shift vasprun with a POTCAR in the same folder.

@MichaelWolloch
Copy link
Contributor Author

MichaelWolloch commented Aug 2, 2023

All in all, I think this PR is ready.

  • Important updates in custodian can now be used because the MaxForceHandler was removed
  • r2SCAN+rVV10 functional should work as expected.
  • requirement files are deleted and setup.py is used exclusively instead. atomate/.github/workflows/test.yml was updated accordingly.

Of course the nmr test will still fail until pymatgen is updated, and there is also the issue of the qchem test, which I am really not qualified to tackle. Maybe @janosh can ping a qchem expert? But this should be a different PR I think.

@janosh
Copy link
Member

janosh commented Aug 2, 2023

@MichaelWolloch This is great! Thanks for keeping atomate up to date with pymatgen and custodian.

I see two tests still failing which is the same number as on the main branch and the changes in this PR are definite improvements so I'm in favor of merging as is @Zhuoying.

Maybe @orionarcher can comment or knows who to ask about the failing QChem test?

    def test_babel_PC_with_RO_depth_0_vs_depth_10(self):
        with patch("atomate.qchem.firetasks.fragmenter.FWAction") as FWAction_patch:
            ft = FragmentMolecule(
                molecule=self.pc,
                depth=0,
                open_rings=True,
                do_triplets=False,
                opt_steps=1000,
            )
            ft.run_task({})
            self.assertEqual(ft.check_db, False)
            depth0frags = ft.unique_fragments
>           self.assertEqual(len(depth0frags), 509)
E           AssertionError: 411 != 509

@Andrew-S-Rosen
Copy link

@samblau and @espottesmith are the main points of contact for Q-Chem and may have insight here.

@Zhuoying
Copy link
Contributor

Hi @MichaelWolloch, Thanks for all the efforts/discussions and diving into the bugfix. Really appreciate for catching this bug and hopping between custodian/pymatgen/atomate to fix.
I am fine with merging it as it is and leaving Q-chem test-fixing in another PR. From what I see now, your PR with pymatgen to fix the POTCAR for chemical shift calculations is still under review @janosh(materialsproject/pymatgen#3204)? I can put an eye on that as well.

@janosh, I retriggered the test; some neb workflow failed due to path_finder in pymatgen has been moved. Do you have an idea where it has been put now so that I can submit another PR to fix?

from pymatgen.analysis.path_finder import ChgcarPotential, NEBPathfinder

When the only failed test is on Q-chem test (which I also don't have any experience with), I will ping Q-chem contact to fix them. Thanks all for providing comments/suggestions under this PR.

@janosh
Copy link
Member

janosh commented Aug 16, 2023

@Zhuoying The same question came up earlier today from @munrojm. According to @shyuep ChgcarPotential, NEBPathfinder were moved to https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion (although I don't see them there).

@MichaelWolloch
Copy link
Contributor Author

MichaelWolloch commented Aug 16, 2023

@Zhuoying The same question came up earlier today from @munrojm. According to @shyuep ChgcarPotential, NEBPathfinder were moved to https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion (although I don't see them there).

I think both are in https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion/pymatgen/analysis/diffusion/neb/pathfinder.py.
NEBPathFinder and ChgcarPotential.

This was done just now: https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion/tree/9e8773f9784772489275738e26ae3a00bae7f16b

@Zhuoying Zhuoying merged commit 33cbc86 into hackingmaterials:main Aug 16, 2023
0 of 2 checks passed
@Zhuoying
Copy link
Contributor

Hi @MichaelWolloch, thanks for the reminder. I have merged your PR.
Changes on the path to NEBPathfinder and pymatgen-analysis-diffusion version (v2023.8.15) are updated.

"pymatgen-analysis-diffusion>=2023.8.15",

Besided qchem and nmr tests, the scan tests are failed as below:
image
I suppose it is related to materialsproject/pymatgen#3204 and would pass tests after this PR merged? I will keep an eye on that if so. Or please ping me to update pymatgen version in setup.py when this is resolved. @janosh Thanks!

@MichaelWolloch
Copy link
Contributor Author

MichaelWolloch commented Aug 17, 2023

Hi @MichaelWolloch, thanks for the reminder. I have merged your PR. Changes on the path to NEBPathfinder and pymatgen-analysis-diffusion version (v2023.8.15) are updated.

"pymatgen-analysis-diffusion>=2023.8.15",

Besided qchem and nmr tests, the scan tests are failed as below:
image
I suppose it is related to materialsproject/pymatgen#3204 and would pass tests after this PR merged? I will keep an eye on that if so. Or please ping me to update pymatgen version in setup.py when this is resolved. @janosh Thanks!

Hi @Zhuoying , and thanks for merging. The SCAN errors are not related to my PR materialsproject/pymatgen#3204, but to a commit from @janosh last week that sets LELF = False in the MPSCANRelaxSet:
materialsproject/pymatgen@1fa96f8

So the reference INCARs for SCAN calculations all need to be updated to have LELF = False. I will do another PR quickly to fix this.

It might be worth considering to change the dependencies actually and fix the current version of at least pymatgen and not use >=?
I am not informed really, but since atomate2 exists, maybe development of atomate is not as active any more and one should "freeze" it a bit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants