membrane CLI support

[atravitz]

Checklist

• All new code is appropriately documented (user-facing code must have complete docstrings).
• Added a news entry, or the changes are not user-facing.
• Ran pre-commit: you can run pre-commit locally or comment on this PR with pre-commit.ci autofix.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

• GPU integration tests
• example notebook testing
• packaging tests: run this for any large feature PRs or PRs that add test data.

Developers certificate of origin

• I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.08%. Comparing base (be6cd88) to head (b2d68a4).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1896      +/-   ##
==========================================
- Coverage   94.62%   90.08%   -4.54%     
==========================================
  Files         215      216       +1     
  Lines       19266    19545     +279     
==========================================
- Hits        18230    17607     -623     
- Misses       1036     1938     +902     

Flag	Coverage Δ
fast-tests	90.08% <100.00%> (?)
slow-tests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[hannahbaumann]

Thanks @atravitz ! I added some comments regarding validation and error handling.
One thing that's currently missing is that the network planner always adds a SolventComponent to the ChemicalSystem, however the membrane case does not support a SolventComponent in the complex.
Once you add the protocol.validate to the network planner, this would also raise an Error.

[hannahbaumann]

Thanks @atravitz , this looks great! Left some small comments, the only important one is the catching of the actual error which would be very helpful to users I think.

[mikemhenry]

We can wait for the dust to settle but I think we should also include a news item here so it shows up in the change log

[mikemhenry]

Maybe add a link in the news item to the membrane prep tutorial?

[hannahbaumann]

Thanks @atravitz , this looks good!
The only thing I just realized is that if the user provides a fully solvated protein-membrane system using the --protein flag, it will create the transformations without an error. This is also not handled in the API, but I wonder if it's even more dangerous on the CLI level. One could e.g. also add a density check on the ProteinComponent and if it's above a threshold, but not a SolvatePDBComponent, at least add a warning? Though people probably don't look at warnings since there are so many. This is probably something that should be handled elsewhere but still wanted to raise it here. Do you have any thoughts @IAlibay ?

[IAlibay]

The only thing I just realized is that if the user provides a fully solvated protein-membrane system using the --protein flag, it will create the transformations without an error. This is also not handled in the API, but I wonder if it's even more dangerous on the CLI level. One could e.g. also add a density check on the ProteinComponent and if it's above a threshold, but not a SolvatePDBComponent, at least add a warning? Though people probably don't look at warnings since there are so many. This is probably something that should be handled elsewhere but still wanted to raise it here. Do you have any thoughts @IAlibay ?

Couple of takes here (already communicated in earlier meeting, just writing it down here):

• It's probably ok to go with this release without that check.
• Adding some check for the component that sees if you have both box vectors and high density would be good. My initial take is that a warning would be better, but I do agree that we just have too many warnings. We could either: 1) give validate an optional "error on warning" that is used by more sensitive tooling like the CLI, or 2) just go with the error and see how many folks it affects.

[hannahbaumann]

Approving this PR to not be blocking the release. I think only the doc string needs an update but since it's in the test it's also not that important.

[github-actions]

No API break detected ✅

[atravitz]

thanks @hannahbaumann! You make a good point, so I opened an issue here to improve validation.