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

Allow floating point numbers for multiplicities #318

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

awvwgk
Copy link
Contributor

@awvwgk awvwgk commented Aug 3, 2023

Closes #317

Description

  • make behavior consistent between molecular / fragment charges and molecular / fragment multiplicities

Changelog description

  • allow floating point numbers for multiplicities

Status

  • Code base linted
  • Ready to go

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #318 (0106856) into master (e942b81) will increase coverage by 0.01%.
The diff coverage is 74.19%.

❗ Current head 0106856 differs from pull request most recent head d623d5f. Consider uploading reports for the commit d623d5f to get more accurate results

Additional details and impacted files

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.

In discussions, @bennybp doesn't anticipate any trouble with the database. Thanks for ensuring existing molecule hashes won't change.

It just occurred to me that qcng harnesses could be collecting a non-meaningful float multiplicity that has heretofore gotten cast to int but will henceforth be float unless we do some processing to cast to int for whole numbers. A lot of molecules in the database could be hard to attach new calcs to. Is this a problem that needs a database migration, Ben?

Otherwise, lgtm!

@loriab
Copy link
Collaborator

loriab commented Sep 30, 2023

@awvwgk, your Union[int, float] construction looked right to me, but upon testing, it was casting values like 3.00001 to 3 in an unpredictable way. I switched the multiplicities to being float like charges are and added a pydantic validator to coerce them to ints where possible.

Note that to create a fractional multiplicity Mol, one has to pass validate=False to bypass the molparse chgmult physics logic. The qcel test suite passes cleanly, and part of psi4's test suite passes cleanly, so I'm reasonably convinced that these changes are harmless. Do they still do what you need?

@awvwgk
Copy link
Contributor Author

awvwgk commented Sep 30, 2023

Why would you have an error for fractional multiplicities when you can pass fractional charges with validation error?

@loriab
Copy link
Collaborator

loriab commented Sep 30, 2023

Why would you have an error for fractional multiplicities when you can pass fractional charges with validation error?

"... without validation error"?

To be clear, mult=3.0 is fine; only values like 3.01 error. From what I recall, the chg/mult logic in molparse wasn't designed for non-int mult or charge. But float charge passes through at least, whereas as-is it errors with float mult -- it would need some changes to accommodate float mult. So how deeply do you want/need fractional quantities incorporated? A small change could probably allow them if fully specified (i.e., no blanks in fragment_multiplicities if nfrag>1 and molecular_multiplicity is fractional).

Comment on lines +313 to +316
if isinstance(molecular_multiplicity, float) or any(isinstance(m, float) for m in fragment_multiplicities):
raise TypeError(
f"validate_and_fill_chgmult() cannot handle fractional multiplicity. m: {molecular_multiplicity}, fm: {fragment_multiplicities}"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not blocking the use of multiplicities like 1.5 I am fine with merging this change. I don't see an equivalent check for charges therefore I am a bit concerned whether this change only in principle allows fractional multiplicities.

@awvwgk
Copy link
Contributor Author

awvwgk commented Jan 15, 2024

Lori @loriab, can we merge this PR or is anything blocking it?

@awvwgk
Copy link
Contributor Author

awvwgk commented Jun 25, 2024

What is the status of this PR, is anything missing?

@loriab
Copy link
Collaborator

loriab commented Jun 28, 2024

Sorry for the delay. So long as what you need is unvalidated pass-through of non-int chg and mult, I think all this PR needs is an extra message for charge like the new warning one for mult in the case of non-integers. I'd better add some non-int charge tests, too. It's true that float charge has been there from the beginning but possibly never exercised.

@awvwgk
Copy link
Contributor Author

awvwgk commented Jun 29, 2024

I think that works for now as a solution

@loriab
Copy link
Collaborator

loriab commented Jul 29, 2024

Ok, loriab@342d7cc is my current proposal for allowing float multiplicity. It's sitting atop #343 for now.

Investigation showed that the validation routine does handle float charges and checking internal consistency between total and fragment charges. It doesn't try to assign fractional multiplicities to go along with fractional charges, and this is probably edge-case enough in QC to not demand those checks. Accordingly, the validation routine has been made float-tolerant for multiplicities, so validate=T/F allowed. It doesn't try to do any fancy guessing, and you may find cases where its handling should be loosened, tightened, or better coordinated with charge in future.

@loriab loriab mentioned this pull request Sep 18, 2024
2 tasks
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.

Floating point number allowed for molecular charge but not molecular multiplicity
2 participants