tBuLi / symfit

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

Minimizer LBFGSB has a bit-string as status_message #325

Closed antonykamp closed 3 years ago

antonykamp commented 3 years ago

Unlike the rest of the minimizers, LBFGSB has a bit-string as status_message. Here is an example:

from symfit.core.support import parameters, variables
from symfit.core.models import Model
from symfit.core.minimizers import LBFGSB
from symfit.core.fit import Fit

import numpy as np

x, y = variables('x, y')
a, b = parameters('a, b')
x_data = np.linspace(1, 10, 10)
y_data = 3 * np.linspace(1, 10, 10) ** 2
model = Model({y: a * x ** b})

fit = Fit(x=x_data, y=y_data, minimizer=LBFGSB, model=model)
fit_result = fit.execute()
print(type(fit_result.status_message)) # <class 'bytes'>

The type is expected to be <class 'str'>. It can be fixed by changing FitResult.

pckroon commented 3 years ago

Or should this be fixed in the LBFGSB minimizer wrapper?

antonykamp commented 3 years ago

A first idea would be an if statement in the FitResult-class, eg:

status_message = message if isinstance(message, str) else message.decode("ascii")

But imo changing the type of status_message afterwards in the wrapper would look more understandable. Because the decoding is in the LBFGSB-class, the relationship between decoding and the minimizer is clearer + other minimizers use this pattern already (eg COBYLA).

class LBFGSB(ScipyGradientMinimize, ScipyBoundedMinimizer):
    """
    Wrapper around :func:`scipy.optimize.minimize`'s LBFGSB algorithm.
    """
    @classmethod
    def method_name(cls):
        return "L-BFGS-B"

    def execute(self, **minimize_options):
        ans = super(LBFGSB, self).execute(**minimize_options)
        ans.status_message = ans.status_message.decode("ascii")
        return ans
antonykamp commented 3 years ago

Funny story: It's already fixed in https://github.com/scipy/scipy/commit/b939e202c60052ea9b7051a1529e0922d70c994a :D Problem: scipy>=1.6 needs python>=3.7 :/

pckroon commented 3 years ago

Personally I'm ok with this, but these kind of things have to go through @tBuLi.

On the other hand, adding an if isinstance(ans.status_message, ByteString): ans.status_message = ans.status_message.decode() is easy and will keep working.

PS Not sure it's ByteString

antonykamp commented 3 years ago

On the other hand, adding an if isinstance(ans.status_message, ByteString): ans.status_message = ans.status_message.decode() is easy and will keep working.

I would prefer this one too :D

tBuLi commented 3 years ago

I agree with the suggested solution, it is easy, cheap and future proof. If you could write a PR @antonykamp, it would be much appreciated!