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.

Sign up for GitHub

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

Jump to bottom

Add LogError to raise LogError for unconverged gaussian log file #1766

Merged
merged 2 commits into from
Nov 6, 2019
Merged

Add LogError to raise LogError for unconverged gaussian log file #1766

merged 2 commits into from
Nov 6, 2019

Conversation

hwpang
Copy link
Contributor

@hwpang hwpang commented Oct 12, 2019

Motivation or Problem

As suggested in #1739

Description of Changes

Add LogError to raise LogError for unconverged gaussian log file

testing

didn't test

@hwpang hwpang requested a review from alongd October 12, 2019 06:49
@codecov
Copy link

codecov bot commented Oct 12, 2019 •
edited
Loading

Codecov Report

Merging #1766 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1766   +/-   ##
=======================================
  Coverage   42.74%   42.74%           
=======================================
  Files          81       81           
  Lines       21078    21078           
  Branches     5491     5491           
=======================================
  Hits         9010     9010           
+ Misses      11067    11054   -13     
- Partials     1001     1014   +13
Impacted Files Coverage Δ
arkane/kinetics.py 12.14% <0%> (ø) ⬆️
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
rmgpy/data/kinetics/database.py 50.61% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 48.35% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
arkane/sensitivity.py 10% <0%> (ø) ⬆️
rmgpy/rmg/input.py 34% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b2555a...919d434. Read the comment docs.

@hwpang hwpang requested review from cgrambow and removed request for alongd October 15, 2019 20:46
alongd
alongd reviewed Oct 16, 2019
View reviewed changes
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks for this fix, @hwpang !
Please add a unit test using an errored Gaussian log file.
While at it, let's raise a similar error in QChem and Molpro Logs. You can find some example crashed jobs here.
(@dranasinghe, could you add a similar line to Orca?)

arkane/gaussian.py Outdated
@@ -247,6 +247,9 @@ def load_conformer(self, symmetry=None, spin_multiplicity=0, optical_isomers=Non
# Read the next line in the file
line = f.readline()

if 'Error termination' in line:
raise LogError('Gaussian log file did not converge.')
Copy link
Member

Choose a reason for hiding this comment

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

Please change the text to f'The Gaussian job in {self.path} did not converge.'

@alongd
Copy link
Member

alongd commented Oct 29, 2019

@hwpang, we'll soon modify log files in Arkane, would you like to merge this PR soon to avoid conflicts?

@hwpang
Copy link
Contributor Author

hwpang commented Oct 30, 2019

@hwpang, we'll soon modify log files in Arkane, would you like to merge this PR soon to avoid conflicts?

Sorry for the slow response. I was really busy last week. I will address the above comments before this Friday.

@hwpang
Copy link
Contributor Author

hwpang commented Nov 4, 2019

Thanks for this fix, @hwpang !
Please add a unit test using an errored Gaussian log file.
While at it, let's raise a similar error in QChem and Molpro Logs. You can find some example crashed jobs here.
(@dranasinghe, could you add a similar line to Orca?)

I've modified the error message and added a unit test for it.

@alongd
Copy link
Member

alongd commented Nov 4, 2019

Great, thanks!
Please rebase and squash the fix commits

@hwpang hwpang force-pushed the addErrorLoadConformer branch from d22604d to f1c4f7b Compare November 4, 2019 21:15
@hwpang
Copy link
Contributor Author

hwpang commented Nov 4, 2019

Great, thanks!
Please rebase and squash the fix commits

I've rebased and squashed the fix commit. Thanks for reviewing!

alongd
alongd reviewed Nov 5, 2019
View reviewed changes
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

It all looks good, thanks. Just a minor request: Could you squash the test commit with the commit that added the unconverged Gaussian log file?

@hwpang hwpang force-pushed the addErrorLoadConformer branch from f1c4f7b to d1310dc Compare November 5, 2019 22:13
@hwpang
Copy link
Contributor Author

hwpang commented Nov 5, 2019

It all looks good, thanks. Just a minor request: Could you squash the test commit with the commit that added the unconverged Gaussian log file?

These two commits were squashed as requested. Thanks!

@alongd
Copy link
Member

alongd commented Nov 6, 2019

Perfect! Rebase again?

@hwpang hwpang force-pushed the addErrorLoadConformer branch from d1310dc to 919d434 Compare November 6, 2019 01:38
@hwpang
Copy link
Contributor Author

hwpang commented Nov 6, 2019

Perfect! Rebase again?

I rebased this branch. Thanks!

alongd
alongd approved these changes Nov 6, 2019
View reviewed changes
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks!

@alongd alongd merged commit 1d4c5cc into master Nov 6, 2019
@alongd alongd deleted the addErrorLoadConformer branch November 6, 2019 03:26
@mliu49 mliu49 mentioned this pull request Nov 26, 2019
Closed
@mliu49 mliu49 mentioned this pull request Dec 4, 2019
Closed
@mliu49 mliu49 mentioned this pull request Dec 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alongd alongd alongd approved these changes

@cgrambow cgrambow Awaiting requested review from cgrambow

Assignees
No one assigned
Labels
Complexity: Low Module: Logging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hwpang @alongd