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

mopac ci lane. pin to qcel 0.25 #372

Merged
merged 4 commits into from
Jul 8, 2022
Merged

mopac ci lane. pin to qcel 0.25 #372

merged 4 commits into from
Jul 8, 2022

Conversation

loriab
Copy link
Collaborator

@loriab loriab commented Jul 7, 2022

Description

Changelog description

EDIT: closes #373

Status

  • Code base linted
  • Ready to go

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #372 (7ed7012) into master (f726364) will increase coverage by 14.79%.
The diff coverage is 100.00%.

@loriab
Copy link
Collaborator Author

loriab commented Jul 7, 2022

@godotalgorithm, any apprehensions about the test changes?

@loriab loriab mentioned this pull request Jul 7, 2022
@loriab loriab added the Testing Related to a testing enhancement or bug. label Jul 7, 2022
@godotalgorithm
Copy link
Member

This looks ok to me - you are now getting MOPAC from conda-forge (which I will be actively maintaining), some reference numbers have been changed, and the "electronic energy" was removed.

The "electronic energy" has been confusing people (it is NOT a total electronic energy for the molecule), so it has been removed from the default output (it is still accessible with a keyword) and I'd prefer not to emphasize it. The forces produced by MOPAC are derivatives of the "heat of formation", so that is the primary energetic quantity of relevance.

The reference numbers (they seem like geometric data for water?) have shifted a bit, but that isn't surprising of an optimized geometry as MOPAC's default geometric convergence isn't that tight and a lot of small changes have happened behind the scenes in the last few years to perturb these results.

@awvwgk
Copy link
Contributor

awvwgk commented Jul 8, 2022

Maybe also update the suggestion on how to install Mopac at

"mopac", return_bool=True, raise_error=raise_error, raise_msg="Please install via http:https://openmopac.net."

@loriab
Copy link
Collaborator Author

loriab commented Jul 8, 2022

This looks ok to me - you are now getting MOPAC from conda-forge (which I will be actively maintaining), some reference numbers have been changed, and the "electronic energy" was removed.

Great! I grouped mopac with the d3/d4 and geometry optimizers for testing, but if you ever find it would benefit from its own testing lane, that can be arranged.

The reference numbers (they seem like geometric data for water?) have shifted a bit, but that isn't surprising of an optimized geometry as MOPAC's default geometric convergence isn't that tight and a lot of small changes have happened behind the scenes in the last few years to perturb these results.

Thanks for the confirmation that accumulated small shifts likely, esp. when combined with geometric algorithms.

@loriab loriab merged commit 7c87644 into MolSSI:master Jul 8, 2022
@loriab loriab deleted the mopac_ci branch July 8, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Related to a testing enhancement or bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mopac test refs
3 participants