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

Support parsing of "final_energy" in Q-Chem 6.1.1 #3580

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Support parsing of "final_energy" in Q-Chem 6.1.1 #3580

merged 4 commits into from
Jan 25, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Jan 24, 2024

Summary

This PR enables proper parsing of "final_energy" in Q-Chem 6.1.1 with QCOutput while maintaining backwards compatability. Partially addresses, but does not fully close, #3578.

CC: @samblau, @espottesmith, @rkingsbury

@janosh --- the error seems unrelated and is in main.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@janosh janosh added fix Bug fix PRs qchem Q-Chem general-purpose electronic structure package labels Jan 25, 2024
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 @Andrew-S-Rosen! 👍

@janosh janosh merged commit 0b8be98 into materialsproject:master Jan 25, 2024
20 of 22 checks passed
@Andrew-S-Rosen Andrew-S-Rosen deleted the qc611 branch January 25, 2024 06:32
assert data["final_energy"] == -76.43205015
qc_out = QCOutput(f"{TEST_FILES_DIR}/qchem/6.1.1.wb97xv.out.gz")
assert qc_out.data["final_energy"] == -76.43205015
n_vals = sum(1 for val in qc_out.data.values() if val is not None)
Copy link
Member Author

Choose a reason for hiding this comment

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

@janosh --- am I understanding correctly that if new properties are added to the .data attribute that this will fail? That seems like it'd be annoying from a CI perspective. For instance, there are definitely others missing that will need to be patched in future PRs.

Copy link
Member

Choose a reason for hiding this comment

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

that's correct. i was thinking it would be good to force future PRs to look at that test and add a line for any new property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs qchem Q-Chem general-purpose electronic structure package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants