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

[BUG] AmberPrm reader reorders atoms when molecules are not contiguous in the file #164

Closed
chryswoods opened this issue Feb 19, 2024 · 3 comments · Fixed by #165
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@chryswoods
Copy link
Contributor

Describe the bug
The AmberPrm reader assumes that molecules in a PRMTOP file are written contiguously. It reorders the atoms into molecule/atom rather than file order when a molecule is not contiguous in the file (i.e. if there are multiple protein chains that are separate molecules, and there are bonded metal ions written at the end of all proteins, but which have bonds to individual protein chains. In this case, the metal ions are moved to appear with their protein, which will be a different order to the file order.

This causes problems when this topology is matched with a coordinate file that does not expect the atoms to be reordered (i.e. a RST or CRD file).

Additional context
This is a bug report generated from files shared separately.

@chryswoods chryswoods added the bug Something isn't working label Feb 19, 2024
@chryswoods
Copy link
Contributor Author

chryswoods commented Feb 19, 2024

(note to self)

A possible solution would be to add a System property that marks the topology as being reordered, and giving hints to the coordinate parser as to how to match the coordinates to the atom order. It would be something like system.property("atom_order"), and would be something like index to mean order by the container's atom index, or number to order by the atom number. The default would be index, with number used in this case.

@chryswoods
Copy link
Contributor Author

(more notes to self)

The change would only need to be made in MoleculeParser (and any frame parsers that use their own logic rather than rely on frame functions defined in MoleculeParser).

The key change would be to inject the reordering at point where frames are pulled from the frame files - the frame should be in an order that matches the index order, meaning re-ordering what is read from the file if the topology was in number order.

The key parts of this class to inject there are;

// first, we are going to work with the group of all molecules, which should

// data in the same order and that this matches the atomidx order

Fortunately, reordering when reading the frame (and before passing the frame to the rest of the code) should be quick, as it will only be done once and the frame then cached. It could fit into the pipeline of aligning and smoothing.

@chryswoods chryswoods added this to the 2024.1.0 milestone Feb 19, 2024
@chryswoods chryswoods self-assigned this Feb 19, 2024
@chryswoods
Copy link
Contributor Author

That helped - I've applied the changes in the latest commit which work as described above.

@chryswoods chryswoods mentioned this issue Feb 21, 2024
@chryswoods chryswoods linked a pull request Feb 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant