Allow floating point numbers for multiplicities #318
Conversation
Codecov Report
Additional details and impacted files |
There was a problem hiding this comment.
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
force-pushed
the
multiplicity
branch
from
ed797b3
to
0106856
Compare
|
@awvwgk, your Note that to create a fractional multiplicity Mol, one has to pass |
|
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 |
| 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}" | ||
| ) |
There was a problem hiding this comment.
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.
|
Lori @loriab, can we merge this PR or is anything blocking it? |
Closes #317
Description
Changelog description
Status