account for charges in convert_mbuild.py

[chrisjonesBSU]

Related to #664 and #678

site.charge defaults to None which might cause issues for some of the writers (e.g. write_gsd)

I think site.charge should default to 0 when not set, and we should look through the convert functions in gmso.external to make sure site.charge is being populated correctly when the information is available.

[chrisjonesBSU]

EDIT: After messing around a bit I answered my own question.

I'm kind of confused how default values are handled with pydantic. Is the correct place to define default values for attributes in the Field class?

For example:

 41     charge_: Optional[Union[u.unyt_quantity, float]] = Field(
 42         None,
 43         description="Charge of the atom",
 44     )
 45 
 46     mass_: Optional[Union[u.unyt_quantity, float]] = Field(
 47         None, description="Mass of the atom"
 48     )

Would become:

 41     charge_: Optional[Union[u.unyt_quantity, float]] = Field(
 42        0.0,
 43         description="Charge of the atom",
 44     )
 45 
 46     mass_: Optional[Union[u.unyt_quantity, float]] = Field(
 47         0.0, description="Mass of the atom"
 48     )

[CalCraven]

As far as I'm aware, this is the change you would want. However, I do think it is better to leave at least the default values to None, same as my comment in Issue #678. This gives us the power to understand if the value has been set to 0 somewhere, or is awaiting some way to get the value. When we read from mbuild, we can populate site.charge as 0 for specific instances if that was a good specific default behavior for a system.

For instance, I think in this function we just need to be checking for charges, and assigning them to the atom.

[chrisjonesBSU]

Yeah, I agree that leaving the default as None provides some extra information and functionality

[daico007]

Yeah, having charge default to None was a conscious decision back then, since charge=0 still carry physical meaning, having that as default may cause unexpected behavior (like folks forget to put charge but everything runs).

[chrisjonesBSU]

Ok, that makes sense. I guess the question remains if from_mbuild should carry over charge and mass information if it's been set in mBuild, or if they should stay as None until the user takes some further action to define them in gmso.

[CalCraven]

If they're there in mBuild, they should be assigned to the GMSO topology. That is certainly the case, not having that converted is a bug.

[daico007]

I think we should carry it over given that those parameters are set in the mBuild Compound (but I am not sure what's the default value for those two fields over mbuild, need to double check). My point of view is that if those values have been set by the user and is part of the particle/bead (not atomtype), they should be brought over and take precedent that the values specified in their atomtypes, but also avoid having default to a logically correct value (0 for charge).

[daico007]

Just checked, and currently, both charge and mass is default to 0 in mbuild, I think we should change those to None

[CalCraven]

That change sounds better to me. It may break some of the current mBuild writers though, so we'll have to keep an eye on that in the PR.

[daico007]

I will start a PR to do that and see how many things break

[daico007]

addressed in mosdef-hub/mbuild#1047 and #680