Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not subtract tolerance from box size #1110

Closed
wants to merge 2 commits into from
Closed

Do not subtract tolerance from box size #1110

wants to merge 2 commits into from

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Nov 15, 2024

Description

Resolves #1106

Checklist

  • Add tests
  • Lint
  • Update docstrings

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.62%. Comparing base (dec585e) to head (deb3e9b).
Report is 96 commits behind head on main.

Additional details and impacted files

@mattwthompson mattwthompson marked this pull request as ready for review November 21, 2024 15:31
@mattwthompson mattwthompson added this to the 0.4.2 milestone Jan 22, 2025
@IAlibay IAlibay mentioned this pull request Jan 28, 2025
@mattwthompson
Copy link
Member Author

@lilyminium any idea if we still want this?

@mattwthompson
Copy link
Member Author

@lilyminium @j-wags do we want this?

@lilyminium
Copy link
Contributor

lilyminium commented Feb 3, 2025

Yes please, I think it still makes sense, despite the original issue being solved.

Unless I'm misunderstanding what the box tolerance should be for, which is 100% a possibility

Edit: I've changed my opinion, please see below.

@Yoshanuikabundi
Copy link
Collaborator

When I wrote this code, I was concerned that packmol might not produce boxes that tile very well, and rather than introduce a second parameter I re-used the tolerance parameter to introduce a small buffer around the edge of the box. This only happens in the call to packmol - the actual returned box should be the requested size or density, just with that small empty buffer on one end of each dimension. In other words, packmol attempts to create a slightly overdense box, which is then placed in the corner of the returned box. After NVT equilibration, the box would be the requested uniform density. Part of my assumption was that PackMOL outputs are really only useful as starting points for equilibration.

@lilyminium
Copy link
Contributor

lilyminium commented Feb 4, 2025

Josh and I just had a quick call where we talked over the above, and we both agreed we'd prefer the option that works generally across systems. Since all the tests I ran to troubleshoot the original packing issue did not implement this fix, that means I have a weak preference to not merge this PR since we know things work without it. I'm happy to re-run tests with this fix to validate if anyone has a strong opinion the other way of course! (And this behaviour in Evaluator seems to work ok)

Edit to clear up my previous confusion over the tolerance parameter -- it defines the space between molecules, and as packmol packs the box fully (?) that means the buffer that Josh has added becomes the space between molecules when tiling.

@mattwthompson
Copy link
Member Author

Thanks all, my preferences here are weak so I'll not go with this PR but make an attempt to document this behavior (whether or not it's different than Evaluator's code)

@mattwthompson
Copy link
Member Author

I will say that it's a little odd that the density is lower because of the 10% scale-up in each linear dimension (33% scaleup in volume) and higher because of how the "tolerance" is subtracted from the edges of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interchange subtracts tolerance when packing boxes, whereas Evaluator didn't.
4 participants