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

Add multipole parsing for Q-Chem IO #3490

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

espottesmith
Copy link
Contributor

Minor PR. Just adding electric multipole (up through hexadecapole) parsing for Q-Chem.

Tests for relevant classes pass locally, black and ruff seem happy enough for the files I changes. Didn't check mypy, so here's praying.

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @espottesmith! 👍 Looking good. Just a few small refactors needed.

Comment on lines 1092 to 1107
{
"XXXX": float(hpole[0]),
"XXXY": float(hpole[1]),
"XXYY": float(hpole[2]),
"XYYY": float(hpole[3]),
"YYYY": float(hpole[4]),
"XXXZ": float(hpole[5]),
"XXYZ": float(hpole[6]),
"XYYZ": float(hpole[7]),
"YYYZ": float(hpole[8]),
"XXZZ": float(hpole[9]),
"XYZZ": float(hpole[10]),
"YYZZ": float(hpole[11]),
"XZZZ": float(hpole[12]),
"YZZZ": float(hpole[13]),
"ZZZZ": float(hpole[14]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be made more concise with a list comprehension

keys = ["XXXX", "XXXY", "XXYY", "XYYY", "YYYY", "XXXZ", "XXYZ", "XYYZ", "YYYZ", "XXZZ", "XYZZ", "YYZZ", "XZZZ", "YZZZ", "ZZZZ"]
hexadecapole = [{key: float(hpole[idx]) for idx, key in enumerate(keys)}]

Likewise above.

@espottesmith
Copy link
Contributor Author

How does this look, @janosh?

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! 👍

@janosh janosh added io Input/output functionality molecules Molecule stuff qchem Q-Chem general-purpose electronic structure package labels Dec 5, 2023
@janosh janosh enabled auto-merge (squash) December 5, 2023 16:41
@espottesmith
Copy link
Contributor Author

I think your most recent proposed changes (particularly the use of str.split() to form the key lists) save a small number of characters at the expense of increasing cognitive load.

As long as this gets merged, I don't really care, but at some point I'd love to chat with you about coding style in PMG. Ultimately code needs to be read by human beings; as such, I strongly believe that efficiency (in terms of LOC or characters or whatever) should not be the ultimate goal.

(For clarity, this critique isn't aimed at your previous requested changes. Using the key lists and enumerate() probably also somewhat increase cognitive load, but in that case I'd say the cleanup was well worth it.)

@janosh
Copy link
Member

janosh commented Dec 5, 2023

@espottesmith Fair enough reverted in 93078ff. I know @shyuep will definitely agree with you there. I like str.split if it avoids multiple line breaks. Personally don't find it hard to read/reason about and the more code fits on one screen height, usually the easier to debug in my experience...

@janosh janosh merged commit 8a4e86d into materialsproject:master Dec 5, 2023
22 checks passed
@janosh janosh added the enhancement A new feature or improvement to an existing one label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality molecules Molecule stuff qchem Q-Chem general-purpose electronic structure package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants