GeomScale / dingo

A python library for metabolic networks sampling and analysis
GNU Lesser General Public License v3.0
41 stars 27 forks source link

Make changes to code style #37

Closed dymil closed 2 years ago

dymil commented 2 years ago

I felt that this pull request ought to be separate from the one just affecting documentation.

There are some inconsistencies which I didn't address, like the placement of braces in function definitions.

The existing (non-Gurobi) unit tests pass, but I don't think those cover this, though most changes should be obviously correct, with the most notable exception being changing the types of psrf_check and parallelism.

vissarion commented 2 years ago

@dymil there are also some conflicts you have to resolve, thanks!

dymil commented 2 years ago

AFAIR the choice of int instead of bool in the binding were because of an issue of Cython to not correctly handle bool types.

If I did it right, I preserved as much of the behavior as possible, even within each function. For example, bool(parallelism) will be True or False as if it were the condition in the old if statements. It will be then be 1 or 0 accordingly as a bint, and that C int will then be implicitly converted to the C++ bool parallelism according to the usual rules.

As an example which captures the above behaviour up until the call from Cython to C++:

from cpython cimport bool
cdef bint a = bool(3)
print(a, <int>(a))
a = bool(None)
print(a, <int>(a))

produces

True 1
False 0

When I print(check_psrf) in slow_mmcs() in volestipy.pyx and when I std::cout<<mmcs_set_of_parameters.psrf_check in mmcs_initialize() in bindings.cpp, things seemed to behave as I expect, i.e., None becomes False becomes 0 when I print the bint and bool, respectively, and [3] becomes True becomes 1.

vissarion commented 2 years ago

Thanks @dymil for the explanations! Please merge with develop to see the tests running on this PR.