WIP: Added ase Nudged Elastic Band in the NewtonNet recipe #2176
Conversation
|
Can one of the admins verify this patch? |
kumaranu
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2176 +/- ##
==========================================
- Coverage 97.41% 97.38% -0.03%
==========================================
Files 85 86 +1
Lines 3518 3673 +155
==========================================
+ Hits 3427 3577 +150
- Misses 91 96 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @kumaranu: I know this is still a WIP but I wanted to provide some initial comments. The biggest thing to note is that in quacc you cannot write things out to the current working directory or any relative paths. Quacc does not cd into the working directory and so there is no guarantee that the current working directory or any relative path is going to be where the runtime/results directory is.
If you are looking to write out files before the calculation completes, then you need write it out based on the tmpdir that is made when quacc.runners.pre.calc_setup is called. For instance, in phonopy, you can see that certain files are written out (e.g. phonopy.yaml, phonopy_auto_band_structure.yaml) based on the tmpdir of the runtime directory:
quacc/src/quacc/runners/phonons.py
Lines 55 to 68 in 04af31e
If you are writing to disk after the calculation completes and a schema is returned, you can query schema["dir_name"] to get the path where the results folder is. In any case, there is going to need to be a fairly significant amount of refactoring here to ensure that your I/O goes where you expect it to go. These functions may have been working fine for you because you are using FireWorks which cd's into a custom directory for each run, but that is very unique to FireWorks. If you were to run these calculations without a workflow engine, for instance, you'll see that the files are not being written out to the unique results folder for that calculation.
Most of your issues in this PR boil down to the fact that you are trying to put too much into the recipes themselves, which are meant to be very well-defined units of work. In reality, most of your additions here should be quacc.runners functions that are called within your new recipes. I/O happens in runners and schemas depending on if it's meant to be done before/during or after the calculation. If using ChatGPT, it has no knowledge of this.
pyproject.toml
Outdated
| @@ -49,7 +49,7 @@ defects = ["pymatgen-analysis-defects>=2023.8.22", "shakenbreak>=3.2.0"] | |||
| jobflow = ["jobflow[fireworks]>=0.1.14", "jobflow-remote>=0.1.0"] | |||
| mlp = ["matgl>=1.0.0", "chgnet>=0.3.3", "mace-torch>=0.3.3", "torch-dftd>=0.4.0"] | |||
| mp = ["atomate2>=0.0.14"] | |||
| newtonnet = ["newtonnet>=1.1"] | |||
| newtonnet = ["newtonnet>=1.1", "geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git"] | |||
There was a problem hiding this comment.
We can't have direct git links in pyproject.toml (unfortunately) because PyPI does not allow us to upload new version of quacc with URLs. The best course of action here is to simply make it clear in the @requires decorator that this package should be installed.
There was a problem hiding this comment.
Hi @Andrew-S-Rosen ,
I want to have tests that run for geodesic interpolate on GitHub. Is there a way you see to do that in case we are not able to put URLs?
Thanks.
There was a problem hiding this comment.
@kumaranu: Simply remove the direct link from pyproject.toml. The requirements.txt files in the tests folder are what is used by CI.
|
I have added a function in runners/ase.py with its test. My test is running alright locally but it is complaining that the required packages are not installed such as "sella", "NewtonNet", "geodesic" etc. Maybe the reason for this problem is that requirements-newtonnet.txt is not called for the packages that are present inside runners/ase.py. But I do not know which file will used to get that list of required packages. Please help resolve this issue. Thanks. Tagging Sam here as I was discussing this issue with him recently. |
I added the following three lines to requirements.txt inside the tests directory to solve that problem: |
| @@ -133,7 +133,7 @@ def test_ase_relax_job(tmp_path, monkeypatch): | |||
| output["parameters"]["orcasimpleinput"] | |||
| == "def2-tzvp engrad normalprint wb97x-d3bj xyzfile" | |||
| ) | |||
| assert output["parameters_opt"]["fmax"] == 0.1 | |||
There was a problem hiding this comment.
Please don't change pre-existing tests. They should pass just fine.
pyproject.toml
Outdated
| @@ -50,7 +50,7 @@ jobflow = ["jobflow[fireworks]>=0.1.14", "jobflow-remote>=0.1.0"] | |||
| mlp = ["matgl>=1.1.2", "chgnet>=0.3.3", "mace-torch>=0.3.3", "torch-dftd>=0.4.0"] | |||
| mp = ["atomate2>=0.0.14"] | |||
| newtonnet = ["newtonnet>=1.1"] | |||
| parsl = ["parsl[monitoring]>=2024.5.27; platform_system!='Windows'"] | |||
There was a problem hiding this comment.
Please don't change unrelated versions. They should work just fine.
There was a problem hiding this comment.
I don't know what was changed here. I see that this file is the same as the one in Quantum-Accelerator: main. Maybe you changed it back already.
There was a problem hiding this comment.
Did I change the parsl version from 2023.10.23 to 2.24.5.27?
There was a problem hiding this comment.
Yes, I took care of it. However, there are several other changes that still need to be reverted back, as shown in the file comparison view.
Although the original problem is solved, I saw two new errors, tblite and mlp tests both of which I did not touch. mlp test were just complaining that they could not find mace. I added the following lines in the requirements.txt to get rid of that error. However, the error in tblite seems much different as the tests ran fine but the numbers don't match. I have left that as it is: |
|
@kumaranu: The I do not quite understand what your goal is here with |
I have removed NewtonNet and Sella from the test_ase.py requirements inside runners directory. It is using BFGS and EMT now. |
|
@kumaranu: Thanks. I will review the PR once you undo all the unrelated changes you've made (there are still many) and remove commented out code blocks. |
src/quacc/runners/ase.py
Outdated
| @@ -320,6 +326,153 @@ def run_vib( | |||
| return vib | |||
|
|
|||
|
|
|||
| def run_path_opt( | |||
There was a problem hiding this comment.
Let's rename this run_neb.
src/quacc/schemas/ase.py
Outdated
| @@ -275,6 +275,40 @@ def summarize_vib_and_thermo( | |||
| ) | |||
|
|
|||
|
|
|||
| def summarize_path_opt_run(dyn: Optimizer) -> OptSchema: | |||
There was a problem hiding this comment.
And then we can rename this summarize_neb_run. It also needs a proper docstring.
There was a problem hiding this comment.
Some more changes to revert later.
tests/core/schemas/test_ase.py
Outdated
There was a problem hiding this comment.
Some more changes to revert later.
There was a problem hiding this comment.
Hi @kumaranu. I have provided an initial review below. There are many areas for cleanup, but overall the logic seems mostly reasonable. If you can address these changes, that would be ideal. Please note that you should also be checking GitHub Actions CI to see what tests fail. You'll see that your docs and pre-commit CI runners failed. Clicking the "details" will show you why.
Once you make these changes, I will review it once more and can assist with remaining issues. Thank you. Please also "resolve" the comments when you are done addressing them.
tests/core/runners/test_ase.py
Outdated
| ) | ||
| ], | ||
| ) | ||
| def test_run_neb( |
There was a problem hiding this comment.
Your test of run_neb should not rely on geodesic-interpolate. We want to make sure that we are only testing one thing at a time (hence the name a "unit test").
There was a problem hiding this comment.
The NEB function needs list of ASE images. I am avoiding it because it will just become a long list of coordinates in this file. Not sure how to do it cleanly.
tests/core/runners/test_ase.py
Outdated
| assert neb_summary["trajectory_results"][1]["energy"] == pytest.approx( | ||
| 1.098, abs=0.01 | ||
| ) |
There was a problem hiding this comment.
A more thorough test of the output schema is required. Also, in general, let's not use such a loose tolerance if possible. This will help us capture errors appropriately.
There was a problem hiding this comment.
Same problem as the geodesic here because I am using geodesic as the precursor to this step.
There was a problem hiding this comment.
There are too many things being tested here at once is the major problem. For instance, runners/test_ase.py should be testing the runner: run_neb. It should be asserting that run_neb() functions appropriately and nothing else. There should be no major precursor to that step and nothing afterwards (i.e. no summarizing).
You'd then make a separate test for summarizing (e.g. tests.cores.schemas) and so on and so forth.
The idea of a "unit test" is that you are testing a specific unit of work.
tests/core/schemas/test_ase.py
Outdated
There was a problem hiding this comment.
Please do not make unrelated changes in your PR like the code changes in this file.
Removed move_magmoms from neb summary function.
fixed some docs.
…overwriting trajectory variable.
|
Hi @Andrew-S-Rosen, Thanks a lot for providing the comments above. I have finished going through them all. As you mention two things where you might need to pitch in:
I have left those two unresolved. Tagging @samblau here just for the update. |
|
Thanks, @kumaranu! I recognize that this has likely taken longer than you anticipated and involved more iterations than desired. That said, I feel that the end result here is much more robust and will ensure that your work can continue to be well-maintained into the future. I am starting my new position Aug. 1st so can't wrap this up immediately, but I will keep it on my to-do list for the near future (middle of August most likely). As you suggested, the two things that need to happen before the final merge are: 1) Ensuring that the |
…ptimization. This was the cause of the error.
Summary of Changes
>>
Target here is to add ase's NEB as the double ended transition state search method for NewtonNet recipe.
<<
Checklist
main.Notes