Update Packmol defaults

[mattwthompson]

Description

Closes #1275

Closes #1349

Checklist

• Improve Packmol defaults
• Use periodic boundary conditions when orthorhombic and 20.15.0
• Pack into specified box, not smaller box, if using PBCs
• Use user-defined/calculated density unmodified prior in packing
• Update tests
• Lint
• Update docstrings
• Update release history

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.84%. Comparing base (6d0d69e) to head (d4e90ed).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
openff/interchange/components/_packmol.py	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1332      +/-   ##
==========================================
- Coverage   93.85%   93.84%   -0.02%     
==========================================
  Files          72       72              
  Lines        6301     6316      +15     
==========================================
+ Hits         5914     5927      +13     
- Misses        387      389       +2     

☔ 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.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Yoshanuikabundi]

This is a great feature, and I'm glad we're doing away with the whole make-the-box-too-big-then-shrink-it thing.

Packmol's new PBC feature only works on orthorhombic boxes: https://m3g.github.io/packmol/userguide.shtml#pbc
(I'm confident this isn't a typo because a triclinic box would need 9 numbers to define origin, three lengths, and three angles, but an orthorhombic box needs only six as the angles are all 90°)

I think there's a little confusion because _build_input_file is designed to build the requested input file, not the correct input file for the topology. That's why the box_size argument takes lengths, not box vectors. I admit that doing the tolerance subtraction bit here rather than in the calling function muddies these waters a bit - sorry about that!

So the wrapper does support triclinic boxes even though Packmol doesn't, and so that support happens mostly outside of _build_input_file.

So _build_input_file probably needs a use_pbcs or use_orthorhombic_pbcs argument, and the calling function needs to set it based on whether it's providing the box to _build_input_file or the brick representation of the box. We need to also make comms clear that orthorhombic boxes are supported, but not general triclinic.

[Yoshanuikabundi]

BLOCKING: We also need a check that the original box (not the box currently passed to this function) is orthorhombic. That'll require some code reorganization, or at least an apply_orthorhombic_pbcs argument to _build_input_file It doesn't make a whole lot of sense to apply orthorhombic PBCs to the brick representation of a triclinic box! Packmol seems to currently only support orthorhombic PBCs.

[mattwthompson]

IIUC this is merged in now

[mattwthompson]

Thanks for the feedback - I suspect it's worth using PBCs where possible and would like to move forward with a similar approach that bakes the orthorhombic condition in. But I'd like to demonstrate that this is worth it before making the change, and unfortunately I'm better off making that change along with changing the default values.

[Yoshanuikabundi]

If by that you mean abandoning support for triclinic boxes, I'd gently push back - I agree accounting for PBCs directly is better, but most of the code I wrote when porting this from IIRC Evaluator was to support triclinic boxes, plus a little glue code for a nicer, topology-focused API. It seems a shame to give up production efficiency in exchange for a somewhat (no-one knows how much?) longer initial equilibration, and it means abandoning the dodecahedral box, which is the new OpenFE default. In practice, I think requiring an orthorhombic box really means requiring a cubic box, except for when there's a bilayer or something else that stops the system from tumbling.

[mattwthompson]

I just mean having the PACKMOL_USE_PBC require the version range and a cubic 90-90-90 box, falling back to existing code otherwise

[mattwthompson]

This is doing everything I want it to do and I believe I've incorporated the limitations found in the first review, so I'm re-opening it. The path has been long but the diff is not so huge

Some details I would like the reviewer to keep in mind

• Are there new tests to be added? Nothing came up, since the existing tests cover all changed behavior
• Code coverage dropped by a tiny bit only because fewer lines are in the denominator - the number of missed lines (numerator) is unchanged
• Docstrings are not rendered in API documentation, so the formatting may be off but that can be fixed later

[Yoshanuikabundi]

This looks great! I think this puts the wrapper in a really good place for stabilization. I only have extremely minor, non-blocking nits.

Docstrings look mostly right, I did find a Markdown-style single backtick codespan repeated a couple times. They can be checked more comprehensively when the module is made public.

Tests look good. I guess I can think of two tests that would be useful (non-blocking):

1. Test that a triclinic box produces the characteristic vacuum region
2. Test that an orthorhombic box doesn't produce the characteristic vacuum region, and perhaps also that there aren't any crazy clashes along the boundary conditions?

Overall I think this is good to go!