search

ISISNeutronMuon / MDANSE

MDANSE: Molecular Dynamics Analysis for Neutron Scattering Experiments
https://www.isis.stfc.ac.uk/Pages/MDANSEproject.aspx
GNU General Public License v3.0
20 stars 4 forks source link

[BUG] Remove dummy atoms during conversion? #352

Closed ChiCheng45 closed 5 months ago

ChiCheng45 commented 5 months ago

Description of the error During the trajectory conversion process dummy atoms are carried on though the MDANSE workflow. So that we get some odd cases where the total DOS includes the DOS of the dummy atoms.

H2O DOS image

H2O DOS (H change to dummy) image

Suggested fix Remove the dummy atoms on trajectory conversion. The user can map the dummy atoms to other atoms with the atom mapping. Alternatively make sure that the dummy atoms are not included in the analysis jobs.

EDIT: I just redid the VACF and it dummy atoms are included in the total VACF so it is consistent with the above.

MBartkowiakSTFC commented 5 months ago

Personally, I would vote for removing the dummy atoms by default. The user will still have a chance to keep them by mapping the dummy type to another type (which, of course, can be another dummy type).

By the way, Atom class has an attribute 'ghost'. Do the ghost atoms get included in the analysis? We probably should check if a) dummy atoms get ghost = True, b) ghost atoms appear in analysis. But I am not sure if that was the purpose of this flag. (To me ghost atoms would be copies of real atoms added for convenience or plotting purposes.)

ChiCheng45 commented 5 months ago

a) dummy atoms are set to ghost = False b) They shouldn't since as far as I can tell ghost atoms shouldn't have a position and other data. I tried loading up an MDT file I edited which a set all atoms but one as ghost which failed since it found more coordinates than it expected.

ChiCheng45 commented 5 months ago

Actually I think there are some cases where we might not want to remove dummy atoms before conversion. For example in the future if we update MDANSE to store partial charges during the conversion process then we may want to include the dummy atoms partial charge to calculate molecule dipoles and etc.

I think we need to come up with a scheme where dummy atoms are not included in some analysis jobs. Also if we include dummy atoms how do we deal with them in #350?

MBartkowiakSTFC commented 5 months ago

Could you add an option to the Atom Selection? At the moment the default setting for atom selection is "all". Could we have some option like "exclude dummy atoms" in the atom selection? Then we would not drop them on conversion, but we would also not include them in analysis unless the user requests is explicitly.

ChiCheng45 commented 5 months ago

Could you add an option to the Atom Selection? At the moment the default setting for atom selection is "all". Could we have some option like "exclude dummy atoms" in the atom selection? Then we would not drop them on conversion, but we would also not include them in analysis unless the user requests is explicitly.

Yea this is a good idea. Will have a look at adding this.