Update nbfix to use parmed structure.copy

mosdef-hub / foyer

A package for atom-typing as well as applying and disseminating forcefields

https://foyer.mosdef.org

MIT License

117 stars 75 forks source link

Update nbfix to use parmed structure.copy #530

Closed jaclark5 closed 1 year ago

jaclark5 commented 1 year ago

PR Summary:

Resolves #529

By using deepcopy() in foyer.utils.nbfixes.apply_nbfix, the parmed structure loses the following attributes ['links', 'symmetry', 'defaults']. Without links, there is an error when copying the structure in mbuild.lammpsdata.

File ".....mbuild/mbuild/formats/lammpsdata.py", line 170, in write_lammpsdata structure = structure.copy(cls=Structure, split_dihedrals=True) File ".....python3.8/site-packages/parmed/structure.py", line 613, in copy for l in self.links:

PR Checklist

[X] Includes appropriate unit test(s)
[X] Appropriate docstring(s) are added/updated
[X] Code is (approximately) PEP8 compliant
[X] Issue(s) raised/addressed?

jaclark5 commented 1 year ago

It seems that there is still failure because the parmed.structure.copy() method doesn't make independent atoms. After looking at that parmed method, it should be a copy... I'm not sure how to resolve this. Parmed version: 3.4.4

daico007 commented 1 year ago

Hmm, I can pull the PR and see what's going with the parmed stuff. The CI tests failing seems to be unrelated to the PR itself (more mamba setup, so probably more with the Github action side, I will just rerun the test in a few hours).

daico007 commented 1 year ago

I pull this local and do some testing, since the CI issue seems to be a bit more complicated than I anticipated. So the only issue is that arise is the original structure (specifically the structure.has_NBFIX(), is somehow also changed so causing one extra step to fail. I suspect some object has been copied by reference (probably not the structure obj itself but probably the atom type object (?)). I will need to try a few things and see if we can avoid this issue.

(Sorry for not having looked at this earlier, things have kinda been hectic)

daico007 commented 1 year ago

A bit more digging indicates that the issue has to due with how parmed structure.copy() works, although the higher level object (like structure, atoms, etc) is a copy (different space in memory), the finer detail (like atom_type) is actually only a copy by reference, so any modification of the atom_type object of the copy structure will also affect the original structure (in our case is the structure.has_NBFIX())

daico007 commented 1 year ago

I opened a PR with parmed (https://github.com/ParmEd/ParmEd/pull/1290) try to address this issue