search

choderalab / espaloma_charge

Standalone charge assignment from Espaloma framework.
MIT License
35 stars 5 forks source link

Is total_charge always assumed to be 0 in the charge() API? #11

Closed jchodera closed 1 year ago

jchodera commented 1 year ago

In the charge(...) API, the total_charge is computed but never used.

When the SPICE run() is called in production.py, the model is called with ChargeEquilibrium() with no kwargs, which means the total_charge could be erroneously assumed to be 0 if total_charge isn't specified and the model doesn't have q_ref.

It's not clear to me that there isn't a code path that mistakenly sets the total_charge to 0. Can we fix this to require total_charge, or eliminate the argument altogether to require that we have the formal charges defined in the graph?

yuanqing-wang commented 1 year ago

Let's simply eliminate the total_charge argument.

jchodera commented 1 year ago

Have you been able to verify that it did not impact SPICE model training or assessment?

yuanqing-wang commented 1 year ago

This function is only used in deployment but not in training or characterization.