split mbis oeprop

[loriab]

Description

This is a start to addressing #2272

Todos

• MBIS_CHARGES and MBIS_VOLUME_RATIOS are now separate oeprop tasks to the user but still reusing code.
• oeprop(wfn, "MBIS_VOLUME_RATIOS") should be fine (indep oeprop fn), but set scf_properties mbis_volume_ratios; energy("scf") will still fail as MBIS fails via QCEngine #2272 reported because those are OEProp class instantiations called from proc.py, and the free atom volumes aren't available. The oeprop.cc code could exit gracefully when free atom volumes aren't available, but having different properties lists for the two calling routes isn't good.
• so why not add the loc from oeprop() to OEProps in proc.py so that atom volumes are available? nice thought, but the fn that produces them itself calls oeprop() and energy() and descends into endless recursion. I haven't sought the logic that makes this all work together.
• add tests. probably some of the existing ones will break for only calling one mbis property but checking volrat.

Checklist

• Tests added for any new features
• All or relevant fraction of full tests run

Status

• Ready for review
• Ready for merge

[JonathonMisiewicz]

The code looks anodyne, but I would appreciate information about how this fixes #2272.

From my understanding, #2272 is that when QCEngine tasks Psi with computing MBIS charges, Psi (a) would try to compute atom volumes but (b) would not have set some necessary variables. I would like to understand why the call coming from QCEngine rather than a Psi input file means we get both (a) and (b) and how this PR disrupts that mechanism.

[loriab]

The code looks anodyne, but I would appreciate information about how this fixes #2272. From my understanding, #2272 is that when QCEngine tasks Psi with computing MBIS charges, Psi (a) would try to compute atom volumes but (b) would not have set some necessary variables. I would like to understand why the call coming from QCEngine rather than a Psi input file means we get both (a) and (b) and how this PR disrupts that mechanism.

• pre-MBIS-volume-capability behavior: requesting the mbis_charges property through set scf_properties ["mbis_chargs"]; energy() route or energy(); oeprop(..., "mbis_charges") route called the "mbis" fns in oeprop.cc which were self-contained like all the other oeprop fns. This is the behavior that MBIS fails via QCEngine #2272 used (former route in particular) and want to use again.
• current / post-MBIS-volume-capability behavior: the "mbis" fns in oeprop.cc now compute two properties -- the original self-contained charges and the volume ratios that need atom volumes for each atom, info that's pre-computed (from energy() calcs) on orders from a line in oeprop() fn. So only the latter route works, and the former route fails for lack of atom volumes. All well and good if you want mbis volumes but a regression if you only care about mbis charges.
• this PR behavior: there are now two oeprop properties and "mbis" fns in oeprop.cc -- one for charges and one for volume ratios. so "mbis_charges" works with both routes (healing 2272), and "mbis_volume_ratios" still works only for the latter route (behavior unchanged, except you don't get volumes for free when request charges).
• future after allow mbis_volume_ratios from scf_properties #2299 addressed: mbis volumes will work with both routes.

Is this any clearer? It isn't the QCEngine involvement that causes trouble -- it's the breakdown in consistency btwn the two calling routes. And if you try the straightforward way of making them consistent (compute free atom volumes in scf_helper fn like the oeprop fn does), you fall into recursion.

[PeterKraus]

Two things:

• it would make sense to check that oeprop(wfn, 'MBIS_CHARGES') works independently from running oeprop(wfn, 'MBIS_VOLUME_RATIOS'); either this test, or mbis-2 below are good candidates.
• the quotation marks are inconsistent - is this also because of black?

[loriab]

good idea -- I've split mbis-1 up now.

the quotes are just my style has changed from single to double to placate black. the driver codebase will get a black run someday, I expect, so I often convert any lines I touch for other reasons to minimize churn at format time. these tests won't likely get that treatment.

[PeterKraus]

The reference test outputs are missing, but apart from that it looks OK so I've approved it.