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

Fix PSF setBox function for Triclinic case #3072

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

andysim
Copy link
Contributor

@andysim andysim commented Mar 17, 2021

When viewing a PDB generated from some CHARMM files earlier, I noticed that the unitcell was orthorhombic even after setting a triclinic box up via the setBox function. Unless I'm misunderstanding, the problem is that the orthorhombic setUnitCellDimensions was being called on the underlying topology object so I switched it. The test case in this PR provides a MWE.

@peastman
Copy link
Member

Thanks! This looks good. I'll merge it once CI finishes running.

@swails
Copy link
Contributor

swails commented Mar 17, 2021

Aaaaand that's because when the CHARMM parsers were written OpenMM didn't support triclinic cells.

ParmEd already does the right thing here. The sticking point to switching to ParmEd's parsers are that I'm backporting the Drude and lone pair support that were added here so we can use ParmEd's parsers as a replacement for both CHARMM and Amber (since the CHARMM parsers here are really just a now-diverged ancestor of what's in ParmEd).

Much of that work is done (ParmEd/ParmEd#1157), and I'm using the test cases here to validate the changes.

Once complete, I'll cut a new ParmEd release and raise a PR here replacing the Amber and CHARMM implementations with what's in ParmEd (as an optional dependency) per the discussion in #3038

@peastman
Copy link
Member

That will be great to have too!

@peastman peastman merged commit 72c70cf into openmm:master Mar 17, 2021
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.

None yet

3 participants