PySCeS / pysces

The official PySCeS project source code repository.
https://pysces.github.io
Other
34 stars 10 forks source link

Python 3: exec() #31

Closed jmrohwer closed 5 years ago

jmrohwer commented 5 years ago

I have created a branch py3 with an initial port. It went better than anticipated. After running 2to3 I had to do some manual fixing, but now all level 1 tests pass!!! So first steps are taken.

I have run into an issue with doMca() though, and it's related to EvalEvar() and EvalEpar(), specifically the helper method __num_deriv_function__(self,x,react,met) on line 5237 in PyscesModel.py. The problem lies in the different ways the exec() is handled in Python 2 (statement) and 3 (function). As a consequence variables are now limited in their scope and no longer passed around, moreover from the documentation of ecec():

Be aware that the return and yield statements may not be used outside of function definitions even within the context of code passed to the exec() function. The return value is None.

See also:
https://stackoverflow.com/questions/15086040/behavior-of-exec-function-in-python-2-and-python-3 https://stackoverflow.com/questions/41808748/exec-python2-vs-python3 https://docs.python.org/3/library/functions.html#exec

Brett, do you have an idea for a fix to refactor this?

jmrohwer commented 5 years ago

Fixed by getting and setting the model attributes directly, no need to use exec!

    def __num_deriv_function__(self,x,react,met):
        """
        __num_deriv_function__(x,react,met)

        System function that evaluates the rate equation, used by numeric perturbation methods to derive
        elasticities. It uses a specific format assigning x to met and evaluating react for v and is tailored for the ScipyDerivative() function using precompiled function strings.

        Arguments:

        x: value to assign to met
        react: reaction
        met: species

        """
        #exec(met)
        #self.Forcing_Function()
        #exec(react)
        #return v
        bk = getattr(self, met) # backup current metabolite value
        setattr(self, met, x)
        self.Forcing_Function()
        v = getattr(self, react)
        return v()
        setattr(self, met, bk) # reset metabolite value
bgoli commented 5 years ago

Excellent work with the conversion and shows that almost 15+ year old code still has some life :-)

With the exec issue I think you may have run into some really ancient code. One thing, in the above code you are returning v() without resetting the variable so what about assigning vout = v() and return that after the reset.

jmrohwer commented 5 years ago

Good point, fixed now with 4995f2a03f72aa609aeb42cfd7fca20e25838292. However, it turns out that after all the years the original code also did not yield the correct elasticities because the metabolites were never reset after the perturbation! It only features in the 4th or 5th decimal but nevertheless. We did not pick it up in the summations because those are always perfect due to calculation by matrix inversion. And I suppose the differences were otherwise too small to pick it up in comparisons with Copasi or other tools.

Thanks for picking this up. It is correct now (double checked by debugging with print statements).