Add perturbation-based quasi-IRC workflows for ORCA and Q-Chem #2042

espottesmith · 2024-04-22T23:23:12Z

Summary of Changes

Title says most of it. I also added a frequency workflow for ORCA; felt strange that there wasn't one already.

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 · 2024-04-22T23:23:14Z

Can one of the admins verify this patch?

espottesmith · 2024-04-22T23:29:04Z

Pinging @Andrew-S-Rosen

codecov · 2024-04-22T23:29:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.12%. Comparing base (23ce801) to head (e20c77e).

❗ Current head e20c77e differs from pull request most recent head 3438be8. Consider uploading reports for the commit 3438be8 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2042   +/-   ##
=======================================
  Coverage   99.11%   99.12%           
=======================================
  Files          81       81           
  Lines        3269     3299   +30     
=======================================
+ Hits         3240     3270   +30     
  Misses         29       29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Andrew-S-Rosen · 2024-04-22T23:55:20Z

src/quacc/recipes/orca/core.py

+    default_inputs = [xc, basis, "normalprint"]
+
+    if numeric:
+        default_inputs.append("numfreq")
+    else:
+        default_inputs.append("freq")


Can make this simpler:

default_inputs = [xc, basis, "normalprint", "numfreq" if numeric else "freq"]

Andrew-S-Rosen

Hi @espottesmith!! Thank you very much for your PR and for making my life easy with this review process.

This is nearly ready to go. I just have a few very minor comments that would be great if you can address. 🙏 Happy to merge afterwards!

Edit: Looks like one of my review comments landed above this message by accident.

Andrew-S-Rosen · 2024-04-22T23:56:07Z

src/quacc/recipes/orca/core.py

+@job
+def ase_quasi_irc_perturb_job(
+    atoms: Atoms,
+    mode: list[list[float]],


Can we move mode to after spin_multiplicity for consistency? This will also match your docstring 😉

It's not immediately clear from the docstring or type hint what "mode" is supposed to be structured as. It might be helpful to say "an NxM matrix where..." in the docstring.

It seems like the type hint is actually list[list[float]] | NDArray where NDArray is taken from from numpy.typing import NDArray.

We can't move it after because charge and spin_muiltiplicity are keyword arguments (following the pattern for other ORCA jobs), while mode is a required argument with no default.

Good point about the type and the docstring, though.

Andrew-S-Rosen · 2024-04-22T23:58:59Z

src/quacc/recipes/qchem/ts.py

+    atoms: Atoms,
+    charge: int,
+    spin_multiplicity: int,
+    mode: list[list[float]],


Same comment here regarding type hint and docstring for mode. 👍

Andrew-S-Rosen · 2024-04-23T00:00:37Z

src/quacc/recipes/qchem/ts.py

+    calc_defaults = recursive_dict_merge(
+        _BASE_SET, {"rem": {"job_type": "force", "method": method, "basis": basis}}
+    )
+    opt_defaults = {"optimizer": Sella} if (Sella is not False) else {}


Mind if we change this to:

{"optimizer": Sella} if has_sella else {}

to match what's in core.py? This would involve having

try: from sella import IRC, Sella except ImportError: Sella = False

changed to

try: from sella import IRC, Sella has_sella = True except ImportError: has_sella = False

Yeah, that seems fine. Mostly I just didn't want to mess with existing code if I didn't have to. New repo for me, and all that.

Andrew-S-Rosen · 2024-04-23T00:01:00Z

src/quacc/utils/coords.py

+    from ase import Atoms
+
+
+def perturb(mol: Atoms, vector: list[list[float]], scale: float):


Can you add a docstring for the function? 🙏

Andrew-S-Rosen · 2024-04-23T00:01:38Z

src/quacc/utils/coords.py

The function you added is best suited in quacc.atoms.core.py. I try to keep utils for boring Python things, e.g. handling lists and dicts and such. The Atoms handling utilities are in quacc.atoms.

Cool. Will move.

Andrew-S-Rosen · 2024-04-23T00:02:48Z

src/quacc/utils/coords.py

+
+
+def perturb(mol: Atoms, vector: list[list[float]], scale: float):
+    mol_copy = copy.deepcopy(mol)


Can you use quacc.atoms.core.copy_atoms() here? There are some rare edge cases where a deepcopy() can yield a crash (e.g. if someone attaches really weird things to the Atoms object... it happens).

Andrew-S-Rosen · 2024-04-23T00:05:35Z

src/quacc/utils/coords.py

+    mode_copy = copy.deepcopy(vector)
+
+    orig_pos = mol_copy.get_positions()
+
+    if not isinstance(mode_copy, np.ndarray):
+        mode_copy = np.asarray(mode_copy)


I don't think you modify vector in-place so there is no need to copy it.

I'd suggest just doing

mode = np.asarray(vector)

and then you can skip both the deepcopy() call and the isinstance check (i..e just call the np.asarray). Note that np.asarray doesn't modify in-place, so you're good.

Andrew-S-Rosen · 2024-04-23T00:06:51Z

tests/core/recipes/orca_recipes/test_orca_recipes.py

+        basis="def2-svp",
+        charge=0,
+        spin_multiplicity=1,
+        nprocs=2,


Would you mind removing nprocs=2 so we can run this on Windows too (i.e. you can remove the decorator afterwards)? Same as below. I don't think nprocs is needed for this test.

Andrew-S-Rosen · 2024-04-23T00:08:35Z

src/quacc/recipes/orca/core.py

+    spin_multiplicity: int = 1,
+    xc: str = "wb97x-d3bj",
+    basis: str = "def2-tzvp",
+    numeric: bool = False,


Mind if we change this to numerical? Feels more boolean to me. (now I'm thinking of alphabet vs alphabetical in my head...).

espottesmith · 2024-04-23T16:28:14Z

Alright, think we're looking good!

Andrew-S-Rosen · 2024-04-23T16:35:09Z

Thank you very much, @espottesmith, for the contribution and addressing the comments! I can confirm this is good to go.

At some point, we will probably want to add a specific test for atoms.core.perturb, but I won't let that hold up this PR.

espottesmith · 2024-04-23T16:36:46Z

Yeah, fair enough. It's a tiny function, and it's indirectly tested by other functions, but a dedicated unit test would be a good idea.

Thanks for your quick response!

espottesmith added 8 commits April 16, 2024 12:17

New Q-Chem QIRC job; needs testing

719832b

Added ORCA QIRC job; also slight reorganization

9a1d296

New ORCA freq job

9037817

Small tweak

0f78d23

Small bugfix

878366f

Q-Chem tests pass; now need tests for ORCA

fdfc395

(very basic) ORCA tests pass

9b8bc28

Updated contributors and recipe list

499fa4e

pre-commit auto-fixes

f2bbce7

espottesmith and others added 3 commits April 22, 2024 16:29

Types

14cad90

Merge remote-tracking branch 'refs/remotes/ewcss/qirc' into qirc

e677191

pre-commit auto-fixes

830d178

Andrew-S-Rosen added the New Recipe label Apr 22, 2024

Andrew-S-Rosen reviewed Apr 22, 2024

View reviewed changes

Andrew-S-Rosen reviewed Apr 23, 2024

View reviewed changes

espottesmith and others added 6 commits April 23, 2024 09:01

Tried to make @Andrew-S-Rosen happy :)

34cf74f

pre-commit auto-fixes

39fd9cb

Flubbed refactor

a551fea

Merge remote-tracking branch 'refs/remotes/ewcss/qirc' into qirc

141745b

Merge branch 'main' into qirc

4c34355

pre-commit auto-fixes

e20c77e

Fix docstring type hint

3438be8

Andrew-S-Rosen merged commit dc41d80 into Quantum-Accelerators:main Apr 23, 2024
16 checks passed

espottesmith deleted the qirc branch April 23, 2024 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add perturbation-based quasi-IRC workflows for ORCA and Q-Chem #2042

Add perturbation-based quasi-IRC workflows for ORCA and Q-Chem #2042

espottesmith commented Apr 22, 2024

buildbot-princeton commented Apr 22, 2024

espottesmith commented Apr 22, 2024

codecov bot commented Apr 22, 2024 •

edited

Andrew-S-Rosen Apr 22, 2024

Andrew-S-Rosen left a comment •

edited

Andrew-S-Rosen Apr 22, 2024 •

edited

espottesmith Apr 23, 2024

Andrew-S-Rosen Apr 22, 2024 •

edited

Andrew-S-Rosen Apr 23, 2024

espottesmith Apr 23, 2024

Andrew-S-Rosen Apr 23, 2024

Andrew-S-Rosen Apr 23, 2024

espottesmith Apr 23, 2024

Andrew-S-Rosen Apr 23, 2024 •

edited

Andrew-S-Rosen Apr 23, 2024

Andrew-S-Rosen Apr 23, 2024

Andrew-S-Rosen Apr 23, 2024

espottesmith commented Apr 23, 2024

Andrew-S-Rosen commented Apr 23, 2024

espottesmith commented Apr 23, 2024

		from ase import Atoms


		def perturb(mol: Atoms, vector: list[list[float]], scale: float):



		def perturb(mol: Atoms, vector: list[list[float]], scale: float):
		mol_copy = copy.deepcopy(mol)

Add perturbation-based quasi-IRC workflows for ORCA and Q-Chem #2042

Add perturbation-based quasi-IRC workflows for ORCA and Q-Chem #2042

Conversation

espottesmith commented Apr 22, 2024

Summary of Changes

Checklist

Notes

buildbot-princeton commented Apr 22, 2024

espottesmith commented Apr 22, 2024

codecov bot commented Apr 22, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Andrew-S-Rosen left a comment • edited

Choose a reason for hiding this comment

Andrew-S-Rosen Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrew-S-Rosen Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrew-S-Rosen Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

espottesmith commented Apr 23, 2024

Andrew-S-Rosen commented Apr 23, 2024

espottesmith commented Apr 23, 2024

codecov bot commented Apr 22, 2024 •

edited

Andrew-S-Rosen left a comment •

edited

Andrew-S-Rosen Apr 22, 2024 •

edited

Andrew-S-Rosen Apr 22, 2024 •

edited

Andrew-S-Rosen Apr 23, 2024 •

edited