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

(WIP) get_program: Added check option and fixed Molpro test jobs #76

Merged
merged 4 commits into from May 28, 2019

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented May 24, 2019

Purposefully only fixed the Molpro test jobs to see if the Terachem ones fail in CI.

@@ -22,7 +22,7 @@ def test_molpro_output_parser(test_case):
data = molpro_info.get_test_data(test_case)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like qcer doesn't populate the list test_case (line 18) for Molpro and Terachem on the CI build. It does run for me locally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I fixed this in qcer, I restarted the tests to see.

@sjrl sjrl force-pushed the get_program branch 2 times, most recently from 9cb5b4c to 62f1c09 Compare May 24, 2019 23:33
@dgasmith
Copy link
Collaborator

Looks like it is picking up on it now, but failing due to the issue that you mentioned that CCSD gradients in Molpro do not export the CCSD total energy to the XML files for some reason.

@sjrl
Copy link
Contributor Author

sjrl commented May 28, 2019

Yes that is correct. I've been meaning to make a fix through that by either doing text parsing or grabbing the final energy from a different place within the xml. Should I do that for this PR?

@dgasmith
Copy link
Collaborator

If you can accomplish your goal via the XML, I say do it. Otherwise I would be a bit hesitant to add text parsing to the clean parser. In this case, you could potentially edit it out the tests records tests.

@sjrl
Copy link
Contributor Author

sjrl commented May 28, 2019

Okay I've updated the output parser to grab the final energy from under the molecule tree if it can't be found in the normal spot under the jobstep tree. For the CI to pass a key had to be removed from qcer which I've done in this PR (MolSSI/QCEngineRecords#12).

@sjrl
Copy link
Contributor Author

sjrl commented May 28, 2019

I've updated the parser so we no longer need qcer PR (MolSSI/QCEngineRecords#12)

Copy link
Collaborator

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

This LGTM. @loriab can you give it a quick once over as well?

Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

lgtm

@dgasmith dgasmith merged commit 69821f8 into MolSSI:master May 28, 2019
@sjrl sjrl deleted the get_program branch May 28, 2019 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants