Skip to content

Conversation

@q-posev
Copy link
Contributor

@q-posev q-posev commented Feb 7, 2023

Description

The purpose of this PR is the following:

The bug observed when optimizing a molecule containing Cl atom using geometric via Psi4.

User API & Changelog headlines

  • Fixing compatibility with geometric 1.0

Dev notes & details

  • The atom list in Psi4 is upper-cased while geometric expects a capitalized one. This is not a problem for atom symbols containing a single letter like H, C etc. but becomes a problem for Cl, Br etc.

Questions

  • Are there any tests of the Psi4/geometric interface?

Checklist

Status

  • Ready for review
  • Ready for merge

@loriab
Copy link
Member

loriab commented Feb 7, 2023

Geometric tests are here https://github.com/psi4/psi4/blob/master/tests/pytests/test_geometric.py , but they wouldn't have caught the problem you identified. If you feel like adding more, go for it, but I don't think this is a problem we're likely to relapse on.

If you wanted to run those, from a compiled psi4 in <objdir>, execute the results of stage/bin/psi4 --psiapi to set up paths, then pytest -v ../tests/pytests/test_geometric.py

@q-posev
Copy link
Contributor Author

q-posev commented Feb 7, 2023

The only way this bug can re-appear is if geometric will change the way they process the atomic labels (which is what happened I suppose). Let me know if a test with Cl atom has to be added.

@loriab
Copy link
Member

loriab commented Feb 7, 2023

The only way this bug can re-appear is if geometric will change the way they process the atomic labels (which is what happened I suppose). Let me know if a test with Cl atom has to be added.

No need, imo. It's psi sending atoms weirdly (for legacy case-code-is-longwinded-in-old-c++ reasons), not geometric expecting atoms weirdly.

@q-posev
Copy link
Contributor Author

q-posev commented Feb 7, 2023

The CI fails in the _________________________ test_qcf_cbs_mbe[snowflake] __________________________ section but I do not think it is related to my fix.

@loriab
Copy link
Member

loriab commented Feb 7, 2023

test_qcf_cbs_mbe[snowflake]

correct, I'm not sure right off if it's a fluke or if qcfractal has changed, but your PR is innocent.

@TiborGY
Copy link
Contributor

TiborGY commented Feb 8, 2023

test_qcf_cbs_mbe[snowflake]

correct, I'm not sure right off if it's a fluke or if qcfractal has changed, but your PR is innocent.

It does not look like fluke, it has been failing on my PRs for a while, eg. #2864

@loriab loriab merged commit 1b46406 into psi4:master Feb 10, 2023
@loriab loriab added this to the Psi4 1.8 milestone Feb 10, 2023
@loriab loriab added bug backport external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC... labels Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport bug external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Atom not in list bug with geometric optimizer

4 participants