joha2 / pyrate

This is my pyrate fork. Goal is for me to provide a useful FreeCAD interface.
GNU General Public License v2.0
3 stars 1 forks source link

Pass in scipy for testability? #15

Closed ghost closed 7 years ago

ghost commented 7 years ago

Just recognized you are using scipy in just a few places in core. Maybe you should allow to pass the fresolve/minimize functions in to open them up for mocking them in your unit tests. This could avoid intermingling tests of scipy functionality with the tests of your own logic.

theinze commented 7 years ago

+1

joha2 commented 7 years ago

@4ourbit thanks for your suggestion! Should placeholder functions substitute fresolve/minimize functions at the places of the code where scipy comes in? Or do you mean we should pass them directly at constructor parameter level? Could you please provide an example? :-)

ghost commented 7 years ago

Constructor level would be possible or passing the function in when optimizing (all Pseudocode):

class ClassWithOptimizableVariables(object):
...
    def optimize(self, func):
        with x0 in self.getActiveVariableValues()
            res = func(x0)
            self.setActiveVariableValues(res.x)
...

The actual optimize function is assembled at top level, e.g. in demo_optimize.py:

from scipy.optimize import minimize
...
s.optimize(functools.partial(minimize, merit.mySimpleDumbRMSSpotSizeMeritFunction, method='nelder-mead', function=osupdate))
...

Whereas in unit tests this optimize function can be replaced with a simpler one (makes tests faster too).

joha2 commented 7 years ago

@4ourbit I just checked the optimization code to see whether there's a method to implement your suggestions best. I saw in the optimizer.py that there is already some separation of Optimizer class from scipy optimization. But I have the feeling that it is not ideal. Do you have an idea how to best decouple both in the optimizer class without making it unusable? :-)

theinze commented 7 years ago

see also https://github.com/theinze/pyrate/issues/4#issuecomment-293904438