search

pyMBE-dev / pyMBE

pyMBE provides tools to facilitate building up molecules with complex architectures in the Molecular Dynamics software ESPResSo. For an up-to-date API documention please check our website:
https://pymbe-dev.github.io/pyMBE/pyMBE.html
GNU General Public License v3.0
5 stars 7 forks source link

Make the ionic product an attribute of pyMBE #48

Closed davidbbeyer closed 2 months ago

davidbbeyer commented 2 months ago

Solves #45 @pm-blanco

pm-blanco commented 2 months ago

@davidbbeyer I think that it would be better to name the varible consistently Kw rather than having different variable names in 

def __init__(self, temperature=None, unit_length=None, unit_charge=None, ionic_product=None)

and in pmb.Kw. Also the name ionic_product sounds a bit vague to me, as it could refer to some other ionic product than that or water (for example for a precipitation reaction).

Could you clean the history of your repository? It seems that you have some old commits from the GCMC implementation that are squashed in the principal repo.

kosovan commented 2 months ago

I strongly support naming the variables as close as possible to the symbols used in Equations.

kosovan commented 2 months ago

I also agree that ionic_product is much more ambiguous than Kw.

davidbbeyer commented 2 months ago

@pm-blanco I have adjusted the nomenclature accordingly. It is probably easiest if you just squash it before merging. I will properly sync my fork after the merge then.

pm-blanco commented 2 months ago

@davidbbeyer the problem with squashing this commits is that then we will have repeated commits in two squashed PR (this one and the previous one) which will mess the history of our repository. I know is a bit tedious, but I think that it would be better if you can solve it before merging this to the main repo.

davidbbeyer commented 2 months ago

@pm-blanco Now it should be cleaned up properly :)