WIP: Added ase Nudged Elastic Band in the NewtonNet recipe

[kumaranu]

Summary of Changes

>> Target here is to add ase's NEB as the double ended transition state search method for NewtonNet recipe.

• I need to make sure that geodesic code is added as a preprocessor for the double ended transition state search.
• NEB will take the output of Geodesic
• Sella will them run from the top point of NEB.
• Tests will need to be added for all the files keeping the coverage of more than 99% before taking it for review.
• Currently, there is a lot of unstructured code in the ts.py and in the test. This will need to be cleaned up to match with the rest of the recipe's in QuAcc.
• The summary dictionaries will need to be written so that the data can be passed back to the database.
• Also, the requirements will be added in the NewtonNet's recipe, however, it is a general problem where ideally the calculator can be changed. If a decision is made later on to move it out of NewtonNet then things will (hopefully) be still usable a general path optimization process and can be added along with the static, relax, freq, and thermo jobs. But, right now, NewtonNet seems like an OK place for this to be put in.

<<

Checklist

• [ ] I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
• [ ] My PR is on a custom branch and is not named main.
• [ ] I have added relevant, comprehensive unit tests.

Notes

• Your PR will likely not be merged without proper and thorough tests.
• If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.
• When your code is ready for review, ping one of the active maintainers.

[buildbot-princeton]

Can one of the admins verify this patch?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 98.17073% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.46%. Comparing base (31484cd) to head (9ee830d). Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/quacc/runners/ase.py	93.75%	3 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2176      +/-   ##
==========================================
+ Coverage   97.44%   97.46%   +0.02%     
==========================================
  Files          85       86       +1     
  Lines        3557     3708     +151     
==========================================
+ Hits         3466     3614     +148     
- Misses         91       94       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[kumaranu]

Hi @Andrew-S-Rosen

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. https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176

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. @samblau

[kumaranu]

Hi @Andrew-S-Rosen

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. https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176

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. @samblau

I added the following three lines to requirements.txt inside the tests directory to solve that problem: newtonnet==1.1.1 geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git sella==2.3.4

[kumaranu]

Hi @Andrew-S-Rosen 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. https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176 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. @samblau

I added the following three lines to requirements.txt inside the tests directory to solve that problem: newtonnet==1.1.1 geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git sella==2.3.4

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. mace-torch==0.3.4 torch-dftd==0.4.0

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: https://github.com/Quantum-Accelerators/quacc/actions/runs/9456768276/job/26049315963?pr=2176

[Andrew-S-Rosen]

@kumaranu: The tests/requirements.txt file cannot have the various packages you have added to it. It is meant to include a minimum set of packages corresponding to the tests-core CI run. If your tests require things like mace, they need to be running with the tests-mlps run. The reason the TBLite issues are happening is because TBLite is not compatible with torch (https://github.com/tblite/tblite/issues/110).

I do not quite understand what your goal is here with tests/requirements.txt. You have added NewtonNet tests, which should only run if NewtonNet is installed.

[kumaranu]

@kumaranu: The tests/requirements.txt file cannot have the various packages you have added to it. It is meant to include a minimum set of packages corresponding to the tests-core CI run. If your tests require things like mace, they need to be running with the tests-mlps run. The reason the TBLite issues are happening is because TBLite is not compatible with torch (tblite/tblite#110).

I do not quite understand what your goal is here with tests/requirements.txt. You have added NewtonNet tests, which should only run if NewtonNet is installed.

I have removed NewtonNet and Sella from the test_ase.py requirements inside runners directory. It is using BFGS and EMT now. @Andrew-S-Rosen @samblau

[Andrew-S-Rosen]

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

[kumaranu]

Hi @Andrew-S-Rosen,

I am not sure why one of my tests is failing on Windows all of a sudden. Please let me know if you see anything obvious to you. Thanks.

[Andrew-S-Rosen]

It looks like a tolerance issue and should be not a problem. You can change the tolerance slightly (just enough for the tests to pass). I am not sure the source either.

[kumaranu]

Hi @Andrew-S-Rosen

I am struggling with the path of traj file for neb. The error is only present on GitHub but not when I run the tests locally. Please let me know if you see any problems over here. https://github.com/Quantum-Accelerators/quacc/actions/runs/10085853797/job/27887414542?pr=2176 I have not gone through all of your comments so I apologize for asking if you already mentioned it somewhere else. Thanks.

[Andrew-S-Rosen]

Hi @Andrew-S-Rosen

I am struggling with the path of traj file for neb. The error is only present on GitHub but not when I run the tests locally. Please let me know if you see any problems over here. https://github.com/Quantum-Accelerators/quacc/actions/runs/10085853797/job/27887414542?pr=2176 I have not gone through all of your comments so I apologize for asking if you already mentioned it somewhere else. Thanks.

@kumaranu: Please see this comment: https://github.com/Quantum-Accelerators/quacc/pull/2176#discussion_r1690015953.

[kumaranu]

Hi @Andrew-S-Rosen I am struggling with the path of traj file for neb. The error is only present on GitHub but not when I run the tests locally. Please let me know if you see any problems over here. https://github.com/Quantum-Accelerators/quacc/actions/runs/10085853797/job/27887414542?pr=2176 I have not gone through all of your comments so I apologize for asking if you already mentioned it somewhere else. Thanks.

@kumaranu: Please see this comment: #2176 (comment).

Did this. All checks are passing now. Thanks.

[kumaranu]

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:

1. run_neb method in the Runner class
2. summarize_opt_run with sumarize_neb_run.

I have left those two unresolved.

Tagging @samblau here just for the update.

[Andrew-S-Rosen]

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 run_neb function becomes a method of the Runner class rather than a standalone function; 2) Ensuring that the summarize_neb_run function become a method of the Summarize class rather than a standalone function. Both scenarios here are meant to minimize code duplication so that changes to one part of the code are properly reflected in your NEB runner and schema. I will ping you when that's done so you can look it over before we wrap this up.

[Andrew-S-Rosen]

Hi @kumaranu! I hope you've been doing well!

In order to get this PR merged, I'd like to ask if you can do a very small number of important tasks.

1. As you can see here, some of your NewtonNet tests run for a very long time (upwards of 4 minutes). This poses some challenges for the automated CI pipeline. Would you be able to take a look at the link (which lists the most timely tests) and reduce the test length? For instance, you might be able to do this by having it only run a few iterations or by using a NEB path guess closer to the final one.
2. It looks like your NEB runner writes out a stray file to the local directory. You can see that in the test I made here. We shouldn't be writing out any files to the current working directory. Could you ensure that this is handled appropriately?

I have refactored some things since we last spoke, so these are the last critical items.

[kumaranu]

Hi @kumaranu! I hope you've been doing well!

In order to get this PR merged, I'd like to ask if you can do a few very small number of important tasks.

1. As you can see here, some of your NewtonNet tests run for a very long time (upwards of 4 minutes). This poses some challenges for the automated CI pipeline. Would you be able to take a look at the link (which lists the most timely tests) and reduce the test length? For instance, you might be able to do this by having it only run a few iterations or by using a NEB path guess closer to the final one.
2. It looks like your NEB runner writes out a stray file to the local directory. You can see that in the test I made here. We shouldn't be writing out any files to the current working directory. Could you ensure that this is handled appropriately?

I have refactored some things since we last spoke, so these are the last critical items.

Hi @Andrew-S-Rosen , I'm sorry for not getting back to you sooner. I will get back to this in the next week or two. Thanks!

[samblau]

hey @kumaranu , any progress on this? Thanks!

[kumaranu]

hey @kumaranu , any progress on this? Thanks!

Hi @samblau , Things have been quite busy lately. Sorry, I couldn't get back to it earlier.

I reduced the timing of the tests by adding max_iter to them and also got rid of writing to the current directory problem. Let's see if it works out OK or not. Thanks!

[Andrew-S-Rosen]

Alright friends, @kumaranu and @samblau. It is time for the merge! Is it perfect? No. Is it good enough? We'll find out!

I did a lot of refactoring here, but things seem to be okay. Let's move.