-
Notifications
You must be signed in to change notification settings - Fork 175
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
Max force handler and r2scan fixes #778
Conversation
…priate for R2SCAN+rVV10)
…e code. Those worked as an input for the removed MaxForceErrorHandler
Had forgotten to change the BPARAM also in the reference INCAR file for |
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.
|
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? |
Feel free to update the req files. No reason to pin to an old Line 1 in f4060e5
|
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, 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 Apparently the change in the Potcar class did break the reading of 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 |
@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 |
I think it somehow has to do with setting charges from POTCAR. At least that what the traceback suggests:
I agree with making a new test using a LCHIMAG=True vasprun.xml. |
…fficial package is broken, so I resorted to using https://pypi.org/project/openbabel-wheel/
Regarding the CI. Apparently the changes to the |
OK, that did not quite do the trick... |
Looks like atomate/.github/workflows/test.yml Lines 41 to 42 in f4060e5
So you need to modify Lines 25 to 43 in f4060e5
|
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 Here is the test in
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 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. |
@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. Please confirm and pay special attention to the end of each atomic data block. |
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? |
@jmmshn, regarding the POTCARs, this is a very good point. The one I tested was just taken from 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 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. |
All in all, I think this PR is ready.
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. |
… to handle all dependencies.
@MichaelWolloch This is great! Thanks for keeping atomate up to date with I see two tests still failing which is the same number as on the 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 |
@samblau and @espottesmith are the main points of contact for Q-Chem and may have insight here. |
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. @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?
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. |
@Zhuoying The same question came up earlier today from @munrojm. According to @shyuep |
I think both are in This was done just now: https://github.com/materialsvirtuallab/pymatgen-analysis-diffusion/tree/9e8773f9784772489275738e26ae3a00bae7f16b |
Hi @MichaelWolloch, thanks for the reminder. I have merged your PR. Line 35 in 52608a1
Besided qchem and nmr tests, the scan tests are failed as below: 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: So the reference INCARs for SCAN calculations all need to be updated to have It might be worth considering to change the dependencies actually and fix the current version of at least pymatgen and not use |
Summary
This fixes 2 bugs that were recently mentioned in issues #776 and #774
MaxForceErrorHandler
for VASP. Also removed associatedmax_force_threshold
(which was used as an input toMaxForceErrorHandler
fromOptimizeFW
andRunVaspCustodian
. Similarly,RELAX_MAX_FORCE
, which was the default formax_force_threshold
was removed fromatomate.vasp.config.py
andatomate.vasp.vasp_config.py
BPARAM
inScanOptimizeFW
from 15.7 to 11.95. This is the right parameter to selectR2SCAN+rVV10
, while the old one was optimized forSCAN+rVV10
, which is no longer used inScanOptimizeFW
.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.