tBuLi / symfit

Symbolic Fitting; fitting as it should be.
http://symfit.readthedocs.org
MIT License
233 stars 17 forks source link

LogLikelihood global fitting support, fixed printing of special functions. #237

Closed tBuLi closed 5 years ago

tBuLi commented 5 years ago

Fixed #234 and Fixed #236.

Unfortunately SymPy printing has to be monkeypatched, I see no other way to solve this. Subclassing just doesn't seem to do the trick.

tBuLi commented 5 years ago

The main issue was that something like using Gamma was not printed to the proper scipy equivalent, because our printer inherited from NumpyPrinter but not from SciPyPrinter. I then tried changing that but that didn't work, and then I found out that the reason why is because the printer you supply to lambdify has to be a LambdaPrinter, which somehow or another has to do some work before the module printers are called. If no printer is provided, modules=['sympy', 'numpy'], and therefore the correct printing is done.

I also tried what is suggested here: https://docs.sympy.org/latest/modules/utilities/lambdify.html, which is to provide a mapping for custom functions along with the modules (ctrl+f "Step 2 is augmented by certain translations" on that page). But in that case I still got the wrong result, so I gave up and monkey patched it.

pckroon commented 5 years ago

I think I understand. In some places we want to replace the LambdaPrinter (step 1 in the lambdify process) to create a different string to evaluate (e.g. matmul), and in other cases we want to affect the mapping from that string to numpy/scipy functions (step 2).

Step 1 we can change by creating a NumpyPrinter subclass, and step 2 we can influence by feeding the right dict to lambdify. Also, I can't find a ScipyPrinter anywhere in the docs.

tBuLi commented 5 years ago

I now remember what my problem was with giving a custom mapping to modules to change certain printing behavior: this dict needs to contain the function that should replace the functionality, not the change in printer method. e.g. {'sin': numpy.sin} will make sin print as a numpy sin, but our case we sometimes make the choice based on certain parameters. e.g. print A**2 using a normal power if it is a 1x1 matrix, but using matrix power if it is larger.

The only way I see is to make a new function that chooses on runtime, but that gives overhead. The current implementation will print it as the correct function.

pckroon commented 5 years ago

Right.

we sometimes make the choice based on certain parameters. e.g. print A**2 using a normal power if it is a 1x1 matrix, but using matrix power if it is larger.

has to be done with a subclass of NumpyPrinter, which affects the string representation we end up with. Then, for things like DiracDelta and probably Besseli, you can use the dict approach to affect how that string gets translated to code.

tBuLi commented 5 years ago

I think I have found a good solution to these problems. The solution is to define _numpycode on the symbolical objects in question, so that this can be picked up by the NumPyPrinter of sympy.

This is the smallest intrusion we can make, because although we will have to slightly monkey patch the relevant objects, it will only affect their behavior under the NumPyPrinter. It will not affect their behavior under other printers, nor will we be tampering with the printers themselves. Additionally, defining this so called printmethod is the solution expected by sympy, since the printer will always check this attribute before trying one of the built in options.

As a small asside, I have decided not to print the DiracDelta anymore for fear of false positives. It is much more useful that users get an error about this delta function being there, so they can reconsider what needs to be done in the first place since you probably shouldn't be having them. I therefore changed the relevant tests to use GradientModel instead of Model. I do think this might also be an issue in sympy, since mathematica does not give delta peaks for second derivatives of Gaussian functions.