Add unit tests for reaction methods

[davidbbeyer]

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]

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

[pm-blanco]

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

[jngrad]

@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]

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]

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

[pm-blanco]

@davidbbeyer The test look good to me, but you forgot to commit your changes to the Makefile, where you have probably added your new unit tests.

There are a few lines of code on those functions that are not currently covered by the new unit tests, most of them related with the use_exclusion_radius_per_type feature and other corner cases (I list them below). You can check the coverage locally following the guidelines in our wiki (this is what I did).

• In setup_cpH, line 2751, we should test a case with use_exclusion_radius_per_type.
• In setup_cpH, lines 2765-2766, just for completion a test case with a pka_set with particles not defined pyMBE (it should produce a warning)
• In setup_gcmc, line 2800, again , we should test a case with use_exclusion_radius_per_type.
• In setup_gcmc, lines 2821 and 2823, we should test cases with ions with the wrong charge to make sure that a value error is raised. You can check testsuite/setup_salt_ions_unit_tests.py line 236 for examples where errors are unit tested.
• In setup_grxmc_reactions, line 2872, case with use_exclusion_radius_per_type.
• In setup_grxmc_reactions, lines 2901-2907 cases with ions of the wrong charge.
• In setup_grxmc_reactions, lines 2993-2994 test case with a pka_set with particles not defined pyMBE (it should produce a warning)
• In setup_grxmc_unified, line 3076 test a case with use_exclusion_radius_per_type.
• In setup_grxmc_unified, lines 3100-3102 cases with ions of the wrong charge.
• In setup_grxmc_unified, lines 3121-3122 case with a pka_set with particles not defined pyMBE (it should produce a warning)

[davidbbeyer]

@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.

[pm-blanco]

Thank you @davidbbeyer, it looks very good to me. Again, it proves how important is to check even the corner cases 😄