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
6 stars 8 forks source link

Add unit tests for reaction methods #85

Closed davidbbeyer closed 3 weeks ago

davidbbeyer commented 3 weeks ago

I have added unit tests for the various functions that set up the reaction methods (pymbe_library.setup_cpH(), pymbe_library.setup_gcmc(), pymbe_library.setup_grxmc_reactions(), pymbe_library.setup_grxmc_unified()). In all cases, the unit test consists of setting up the corresponding reactions for a system containing a weak acid and base in espresso and explicitly checking that the number of reactions is correct. Furthermore, the values of the equilibrium constants of the reactions are checked to see if they are correct.

Additionally, I have also added a unit test that checks if the function pymbe_library.determine_reservoir_concentrations() returns the correct reservoir concentrations for an ideal system. The predictions are checked over a wide range of pH-values and salt concentrations by directly comparing it to the values obtained by a calculation independent from pyMBE.

davidbbeyer commented 3 weeks ago

There seems to be some problem with the pipeline...

pm-blanco commented 3 weeks ago

@jngrad it seems related again with EESI and espresso 4.2.2, could you take a look?

jngrad commented 3 weeks ago

@jngrad it seems related again with EESI and espresso 4.2.2, could you take a look?

This is a harmless EESSI warning. The execution continued and failed while running pylint: values were stored in variables that are never read. The usual way if discarded unwanted values from a function that returns a tuple is to store the values into placeholder _ variables, like so: a, _, _b = (1, 2, 3) or a, *_ = (1, 2, 3, 4, 5, 6). See PEP 3132 for more details. Although _ is a special variable name that can be used like the ANS key on a pocket calculator, this is a niche feature that only exists in interactive interpreters, so it's safe to use _ in a script that is imported or executed.

pm-blanco commented 3 weeks ago

Oh, I am sorry for the misleading comment. I am travelling and I checked this too quick, Thanks @jngrad. Could you please fix it @davidbbeyer ? I will check this PR more throughly soon.

davidbbeyer commented 3 weeks ago

Thanks @jngrad and @pm-blanco, now it is working

davidbbeyer commented 3 weeks ago

@pm-blanco I have implemented all of your suggestions and checked that the various functions for setting up the reactions have full coverage now. Also, while implementing the additional test, I realized that the function get_radius_map returns a dict with the radius in terms of units (using pint). This was causing problems when using the reaction methods with the option use_exclusion_radius_per_type, which, however, seems to be used nowhere in the repository (until I added a unit test). I have now simply added an optional argument dimensionless to that function, so one can set if the returned dictionary contains radii with units or just numbers.