Add QMMM functionalities, add TODOs for bugs #19
Conversation
| # Second geometry for ci_vec_overlap job | ||
| # TODO: Not tested yet! | ||
| try: | ||
| geom2 = atomic_input.keywords.pop("geom2") | ||
| if isinstance(geom2, ndarray): | ||
| geom2 = geom2.flatten() | ||
| if len(atomic_input.molecule.symbols) != len(geom2) / 3.0: | ||
| raise ValueError( | ||
| "Geometry provided to geom2 does not match atom list" | ||
| ) | ||
| del ji.xyz2[:] | ||
| ji.xyz2.extend(geom2) | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
Is there a way we can nest this logic under some sort of if ci_vec_overlap_job is True sort of statement? That way we only execute this logic when explicitly declared by the caller instead of implicitly when certain values get passed? That can help avoid issues where accidentally passed values create weird bugs that are implicit and hard to track down :)
|
|
||
| # MM atom geometries | ||
| try: | ||
| mm_geom = atomic_input.keywords.pop("mm_geometry") | ||
| except KeyError: | ||
| mm_geom = None | ||
| try: | ||
| qm_indices = atomic_input.keywords.pop("qm_indices") | ||
| except KeyError: | ||
| qm_indices = None | ||
| try: | ||
| prmtop = atomic_input.keywords.pop("prmtop") | ||
| except KeyError: | ||
| prmtop = None | ||
|
|
||
| if (mm_geom is None) and (qm_indices is None) and (prmtop is None): | ||
| # No QMMM | ||
| pass | ||
| elif (mm_geom is not None) and (qm_indices is not None) and (prmtop is not None): | ||
| if isinstance(mm_geom, ndarray): | ||
| mm_geom = mm_geom.flatten() | ||
|
|
||
| del ji.mmatom_position[:] | ||
| del ji.qm_indices[:] | ||
| ji.mmatom_position.extend(mm_geom) | ||
| ji.qm_indices.extend(qm_indices) | ||
| ji.prmtop_path = str(prmtop) | ||
| else: | ||
| raise Exception("QM/MM parameters are not set correctly, " | ||
| "either one of \"mm_geometry\", \"qm_indices\" or \"prmtop\"" | ||
| "was not set!") |
There was a problem hiding this comment.
Same here as above comment. Can this be encapsulated in a single function that executes if some QM/MM variable is True/False instead of being an implicit statement that executes if certain values are populated on the job input?
| # TODO: Gradient calculation should also return energy. Henry throw energy and gradient | ||
| # outputs to extras["qcvars"]. There should be a cleaner way. |
There was a problem hiding this comment.
It does return energy in the result.properties.return_energy value.
There was a problem hiding this comment.
I am trying to follow the specification created here as much as possible so that we have a result that is interoperable with other QC packages. https://github.com/MolSSI/QCElemental/blob/ec9d754414824e129729f477e6077a14d859dc85/qcelemental/models/results.py#L543
| @@ -121,6 +169,8 @@ def job_output_to_atomic_result( | |||
| atomic_result.extras.update( | |||
| { | |||
| "qcvars": { | |||
| "energy": jo_dict.get("energy"), | |||
There was a problem hiding this comment.
This should likely be removed as per the note above re: return energy being on result.properties.return_energy
| @@ -121,6 +169,8 @@ def job_output_to_atomic_result( | |||
| atomic_result.extras.update( | |||
| { | |||
| "qcvars": { | |||
| "energy": jo_dict.get("energy"), | |||
| "gradient": jo_dict.get("gradient"), | |||
There was a problem hiding this comment.
Take a look at the full results specification at: https://github.com/MolSSI/QCElemental/blob/ec9d754414824e129729f477e6077a14d859dc85/qcelemental/models/results.py. Throwing things into qcvars should be a last result if there isn't a fully structured way to specify a value. I've likely overused the qcvars field because I didn't know what half these values meant, but I'd like to migrate value to their proper field as I learn what they actually are and where they actually belong on an AtomicResult object.
There was a problem hiding this comment.
That is to say, if there's a better field for this (and I know one field is the result.return_valueif gradient was the requested computation, as per: https://github.com/MolSSI/QCElemental/blob/ec9d754414824e129729f477e6077a14d859dc85/qcelemental/models/results.py#L555-L558), let's put it there! However, if you are trying to return a gradient when this wasn't the primary computation requested maybe qcvars is all we got??
| "mm_gradient": jo_dict.get("mmatom_gradient"), | ||
| "dipole_moment": jo_dict.get("dipoles")[3], | ||
| "dipole_vector": jo_dict.get("dipoles")[:3], |
There was a problem hiding this comment.
Dipole moment: https://github.com/MolSSI/QCElemental/blob/ec9d754414824e129729f477e6077a14d859dc85/qcelemental/models/results.py#L69
Any chance these values match a field found in the results.py in QCElemental?
| @@ -1,76 +1,97 @@ | |||
| import os | |||
There was a problem hiding this comment.
Don't understand the changes made to the test here, just flagging it for conversation later :)
There was a problem hiding this comment.
Ok, comments attached! Looks pretty good.
Most comments revolve around:
- Seeing if there's a structured output location for results we can find in https://github.com/MolSSI/QCElemental/blob/ec9d754414824e129729f477e6077a14d859dc85/qcelemental/models/results.py before resorting to the
qcvarsfield. - Nesting some of the logic for extracting values from the
AtomicInputobject under explicit keywords that "activate" a QM/MM job. - Understanding the test changes you made to
test_ci_overlap.py
Looking forward chatting more :)
No description provided.