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

Update ORCA to work with ASE v3.23 #333

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Update ORCA to work with ASE v3.23 #333

merged 8 commits into from
Aug 29, 2024

Conversation

gelzinyte
Copy link
Contributor

Some notes on the changes:

  • Update to work with the ASE's "profile" update
  • The old code was re-implementing ase.orca.ORCA.calculate. This PR instead calls the parent self.calculate() and all the modifications are moved in before and after. These are
    • Fill in any missing parameters with the default values. (The default values are unfortunately copy-pased from ase's orca.py, so can fall out of date, but ase does not expose them.) ASE does that here, but at the time the input file is written. The default parameters don't include an engrad keyword that turns on the forces calculation.
    • Enforce the force calculation by adding an engrad keyword. Currently, if it's not supplied by the user, get_forces() either fails or runs orca for a second time.
    • Get default singlet/doublet multiplicity if none is given.
    • Check convergence. Currently, ase raises no error if "Wavefunction is not converged" is found in the output here.
    • Read geometry optimised structure and optimisation trajectory, if the keyword is supplied by the user (a wfl-only extension)
    • apply a user-supplied post-processing structure (e.g. use an external program to extract charges with natural population analysis)
  • Update unit tests. In particular, run the calculation and compare the results to reference values, instead of parsing output from saved orca outputs without running the program itself.

The documentation example and frequency reading still need updating, but I would like to contribute those in a separate PR, so that the major fixes aren't any more delayed.

All unit tests pass locally, except for the one marked with an x-fail.

@bernstei how does this look?

@gelzinyte
Copy link
Contributor Author

No idea why there's a problem with the unit tests' torch set up, locally that bit executes without a problem.

@gelzinyte gelzinyte mentioned this pull request Aug 16, 2024
3 tasks
@bernstei
Copy link
Contributor

bernstei commented Aug 16, 2024

No idea why there's a problem with the unit tests' torch set up, locally that bit executes without a problem.

It's a pip version issue, which is fixed in #332 (or, more accurately, should be, but I'm waiting for my suggested fix to be tested)

@bernstei
Copy link
Contributor

@gelzinyte I just merged #330, so maybe merge from main into this branch, so we can confirm there are no bad interactions?

@gelzinyte
Copy link
Contributor Author

It's a pip version issue, which is fixed in #332 (or, more accurately, should be, but I'm waiting for my suggested fix to be tested)

Ah, missed that. Let's hear from that PR before fixing these tests.

@gelzinyte
Copy link
Contributor Author

@gelzinyte I just merged #330, so maybe merge from main into this branch, so we can confirm there are no bad interactions?

The local orca tests still pass ok after the merge

@bernstei
Copy link
Contributor

bernstei commented Aug 29, 2024

@gelzinyte can you merge from main again, to see if it fixes the CI torch error and all the tests pass?

Actually, since this is a branch in the main github, I can try to do it.

@bernstei bernstei merged commit c59c86d into main Aug 29, 2024
5 checks passed
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.

2 participants