Issue #162 - scale box factor change

[IAlibay]

This PR, similar to the upstream Interchange v0.5+ behaviour, introduces a breaking change when packing systems where the number of solvent molecules is defined.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.10%. Comparing base (0ac3cb7) to head (a5003d6).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   96.10%   93.10%   -3.01%     
==========================================
  Files          35       35              
  Lines        3262     3262              
==========================================
- Hits         3135     3037      -98     
- Misses        127      225      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[IAlibay]

Should it be or should it not?
I.e. 50% of the times it should be one and 50% of the time it should not.

[j-wags]

To ensure I understand before I misspeak/go down a rabbit hole - Could you clarify that the 50-50 comment means that

• some code paths would need this default set to 0.715 to maintain the same default behavior given the box_scaleup_factor=1.0 change,
• other code paths would maintain the same behavior without this being changed and would have different default behavior if this change is made

[IAlibay]

Yes, when you define number_of_solvent_molecules which happens for non-water SFEs (e.g. MNSOL benchmarks), then you need to adjuste the target density to 0.715 unlike the previous 0.95.

However, when you don't (because you are instead just defining a solvent padding), which happens with water solvent SFEs (e.g. freesolv benchmarks), then the current 0.95 target density is correct and shouldn't be changed.

This matches the behaviour introduced in Interchange v0.5, where some code paths that didn't calculate the box density from the number of molecules (e.g. solvate_topology and solvate_topology_nonwater) wouldn't have been affected by the scale change and therefore wouldn't need their target density adjusted.

[j-wags]

Short version:

I've read through this and traced the code paths, and I agree with @IAlibay's assessment of which behaviors would change and which of our use cases would be affected. I think that changing the default value of target_density to 0.715 is the better option.

If my understanding is correct, changing the default should cause several tests to fail until they have target_density=0.95 set. This corresponds to a change we'll need to make in our FreeSolv submission scripts (I'll ensure this change gets passed on to @Yoshanuikabundi and @lilyminium)

I approve this PR pending changing the default to target_density=0.715 and correcting the tests that fail as a result. Also happy to re-review after the changes are made if that's desired.

Long version: (not necessary to read, mostly for me if I need a refresher on this)

Our current MNSol workflow defines PackmolSolvationSettings.number_of_solvent_molecules and PackmolSolvationSettings.solvent_padding, and gets the default value of PackmolSolvationSettings.target_density.

Our current FreeSolv workflow defines PackmolSolvationSettings.solvent_padding and gets the default value of PackmolSolvationSettings.target_density

The purpose of _process_solvation_inputs is to take whatever information about the number of mols/density/box size the user provided in the SolvationSettings object and use it to define the required inputs for calling PackMol (box_shape, n_solvent, and box_vectors). Some of these values can be defined explicitly in the SolvationSettings object and those will be passed through unchanged, with the other values calculated assuming those fixed values.

If SolvationSettings.number_of_solvent_molecules is defined, then _process_solvation_inputs calls _box_density_from_mols, which always has its behavior changed by the box_scaleup_factor=1.0 change in this PR. This would affect our MNSol workflow but not the FreeSolv workflow.

The effect on the MNSol workflow can be counteracted by scaling the value of PackmolSolvationSettings.target_density to compensate (by a factor of 1.1^3). However this change in the default values would then affect the FreeSolv workflow, unless it manually sets a scaled value of PackmolSolvationSettings.target_density.

So the remaining question is whether to change the defaults or not.

Is there a way to make this fix without causing a behavior change for either workflow?

Not reasonably. Because _process_solvation_inputs doesn't know whether the target_density value it sees is coming from the default value of the PackmolSolvationSettings object or a user-supplied value, there would be an unreasonable amount of complexity involved here. And keeping things the same by modifying PackmolSolvationSettings would require it to change the values it returns in an attempt to counteract some logic that's outside itself (the conditionals in _process_solvation_inputs)

Which tests need to be updated?

A test is affected by the box_scaleup_factor=1.0 change if it manually sets XSolvationSettings.number_of_solvent_molecules.

The current changes in this PR only target the tests that DO define number_of_solvent_molecules, which is correct if the default value of target_density isn't changed.

If the default value of target_density is changed to 0.715, then the current changes to the tests should be unnecessary, however several (all?) OTHER tests will need to have their target_density values set to 0.95.

It's probably a generally good thing to specify target_density for most tests, so there's no harm in keeping the current changes to tests, but if we change the default then the ones that weren't changed should be set to 0.95.

Are changes needed in tests that use OpenMM solvation?

Both openmm_solvation and packmol_solvation call _process_solvation_inputs using the SolvationSetting they're given as input, so both should be affected by this change. This means that calls to OpenMM solvation which provide a number_of_solvent_molecules argument will also be affected.

Pros and cons of changing/not changing default target_density

Pro "keep default target_density=0.95":

• Preserves behavior for cases where number_of_solvent_molecules IS NOT provided

Pro "change default target_density to 0.715":

• Preserves behavior for cases where number_of_solvent_molecules IS provided
• Avoids the problem where attempting to use the OLD default value of target_density=0.95 when number_of_solvent_molecules is also defined and the Packmol backend is selected would lead to really slow packing (since it tries to achieve a higher effective density).

With that last point being the only effective differentiator, I'm in favor of changing the default target density to 0.715.

[j-wags]

(not blocking) Rewording with a bit more detail, if desired

Suggested change

	To roughly reproduce previous behavior, we recommend reducing the
	``target_density`` to 0.715 * unit.grams / unit.mL when
	defining ``number_of_solvent_molecules``.
	If you were calling PackmolSolvation before with a defined
	``number_of_solvent_molecules`` and not providing
	``target_density`` (default value 0.95 g/mL), then to recover
	the same behavior you should manually set the target density
	to 0.715 g/mL (0.95 x (1 / (1.1^3)). If you WERE manually setting
	a ``number_of_solvent_molecules`` AND ``target density``,
	then you should scale the ``target_density`` by 0.751 (1 / (1.1^3))
	to recover the old behavior.

[j-wags]

(blocking)

Suggested change

	target_density: GramsPerMolQuantity | None = 0.95 * unit.grams / unit.mL # noqa: F821
	target_density: GramsPerMolQuantity | None = 0.715 * unit.grams / unit.mL # noqa: F821