model: use guess method where present to automatically populate parameters when appropriate

[jagerber48]

Not necessarily something I think should definitely be in lmfit, but curious to have a discussion to see if it makes sense. A nice behavior would be that if I try to perform a fit with a model that the model auto-populates any parameters I have explicitly supplied using the model's guess method. I can see that these parameters get auto-populated now already but I'm not sure where their values come from. Would this behavior conflict with other existing functionality? Maybe parameter hints (I don't understand these yet)?

[newville]

@jagerber48 Models are independent of particular parameter values, so that it is really the combination of a Model (an idealized "Guassian" or "Decaying Sine Wave") and set of independent data, and a set of Parameter values that give a representation of a dataset: "A Gaussian centered at 4000 with a width of 12" or "A Decaying Sinewave with frequency of 1030 kHz", for example.

Model.guess generates a set of Parameters with particular values. You can then use those Parameter values to evaluate the Model.

Parameter hints are really intended to indicate characteristics of Parameters for a Model in order for the Model to make sense. In general, Parameters do not need boundaries, but for most of the Peak-like models there are Parameters that correspond to a width (typically called sigma). For a peak, it just makes no sense for the width to be negative. So, a Peak-like Model might want to say that sigma > 0. That is sort of independent of whether sigma is on the order of 0.001 or 50,000. That is really what Parameter hints are intended for. They can also be useful to set expressions for derived parameters (say, FWHM given sigma, or period given frequency, etc -- not hard but nice to build in). Many people abuse them, but we're not going to encourage that.

When you do a fit, the result includes update parameter values, so that you can compare the initial parameters and the best-fit parameters.

[jagerber48]

@newville
Right now I can do

params = model.guess(y, x=x)
result = model.fit(y, x=x, params=params)

But the suggestion is to be able to do:

result = model.fit(y, x=x)

with the same result. Right now the second version runs, but seems to populate the initial parameters with some default value for the model. I guess my suggestion is that, rather than use default parameters which may or may not exist and which have no relation to the data set x, y that was passed in, the model could use its guess function to populate any unprovided parameters. The key is that the fit method requires the data to be input so it has the information it needs to call guess().

Basically if a parameter is not supplied by the user then it uses the guess value (extracted by running model.guess(y, x)). If not guess value is provided then perhaps there could be a fallback on a hardcoded default value. If no default value is provided then maybe it could fallback on a value of 1.

Alternatively, if excluding parameters in fit requests is to be discouraged, maybe ALL implicit auto-population of parameters should be removed... But that doesn't seem user-friendly.

[newville]

@jagerber48

Right now I can do

params = model.guess(y, x=x)
result = model.fit(y, x=x, params=params)

But the suggestion is to be able to do:

result = model.fit(y, x=x)

with the same result.

I would be very much against that. If the user does not pass in parameters, the initial values should not be guessed automatically. The values will take the default values defined by the model.

Right now the second version runs, but seems to populate the initial parameters with some default value for the model. I guess my suggestion is that, rather than use default parameters which may or may not exist and which have no relation to the data set x, y that was passed in, the model could use its guess function to populate any unprovided parameters. The key is that the fit method requires the data to be input so it has the information it needs to call guess().

I am strongly opposed. There should be less magic in setting Parameters and more enforcement of creating parameters. If a user wants to use Model.guess, they can.

Basically if a parameter is not supplied by the user then it uses the guess value (extracted by running model.guess(y, x)). If not guess value is provided then perhaps there could be a fallback on a hardcoded default value. If no default value is provided then maybe it could fallback on a value of 1.

I am strongly opposed to having a finite default value for parameters. It is OK if a model function sets a default for some parameters. If we are ever in a position where we are going to set initial values because neither the user nor the person who wrote the model gave one, the only values I would be in favor of using are -np.inf or np.nan.

Alternatively, if excluding parameters in fit requests is to be discouraged, maybe ALL implicit auto-population of parameters should be removed... But that doesn't seem user-friendly.

I would be strongly opposed to removing parameters.

[jagerber48]

I sympathize with the worry about "There should be less magic in setting Parameters and more enforcement of creating parameters."

But then I'm puzzled about why parameters are automatically (magically) populated with default parameters. I would strongly prefer guess parameters to arbitrary default parameters. And if I can't have guess parameters I would prefer an exception being raised to default parameters that have nothing to do with my dataset.

At the end when I suggest removing all implicit auto-population of parameters you say "I would be strongly opposed to removing parameters". Is this because you think there is some benefit to default parameters, or because removing default parameters would break backwards compatibility?

If you think there is virtue in automagically seeding with default parameters, then I'd suggest there's more virtue in automagically seeding with guess parameters that are aware of the dataset. The downside of automagically seeding with guess parameters is that the guess function may be costly. Perhaps another suggestion is a flag in the fit method that exposes the behavior I'm suggesting?

[newville]

@jagerber48

But then I'm puzzled about why parameters are automatically (magically) populated with default parameters.

They are only given default parameters if those are in the definition of the model function. So, as if the person writing the model wanted there to be default values.

I would strongly prefer guess parameters to arbitrary default parameters. And if I can't have guess parameters I would prefer an exception being raised to default parameters that have nothing to do with my dataset.

If you strongly prefer using guessed parameter values, then guess parameter values and use them. Good choice. Don't rely on that being done automatically. Sometimes the guesses are not very good. There should be a step in your code between "guess values" and "use those guessed values".

If you think there is virtue in automagically seeding with default parameters,

I do not think that. I think automagically setting values is the opposite of a virtue.

then I'd suggest there's more virtue in automagically seeding with guess parameters that are aware of the dataset.

Initial values for Parameters are always dependent on the data and it is always the responsibility of the user to provide initial values. One of the most frustrating parts about supporting this library or helping people with scipy.optimize.curve_fit is the very high number of people who are unwilling to understand this.

The downside of automagically seeding with guess parameters is that the guess function may be costly.

The downside of magic is the illusion.

Perhaps another suggestion is a flag in the fit method that exposes the behavior I'm suggesting?

I would not be in favor of that.

The only change I might ever favor would be to have initial default values be np.nan or -np.inf unless explicitly set by the user when creating parameters. Currently, parameter values that are unspecified (by the input Parameters or function definition) do get an initial value of -np.inf. All fits would then immediately fail.

Changing from using initial values as set in the function definition to ignoring initial values set in the function definition and setting those values to -np.inf (including for the sort of "optional variables") would probably break many existing scripts, and not be widely seen as a positive change.

I'm okay with the code as it is.

[jagerber48]

I'm fine with doing less magic, so if automatically implementing guesses is off the table the fine.

I guess I would reframe the discussion then. See this code at the top of Model.fit()

lmfit-py/lmfit/model.py

Lines 1008 to 1011 in 502cc9b

	if params is None:
	params = self.make_params(verbose=verbose)
	else:
	params = deepcopy(params)

I feel like you already addressed by points below, but now that I understand you stance I just want to clarify directly:
Why not make params a required argument to Model.fit()? It seems that that would be most in the spirit of "enforcement of creating parameters".

If the entirety of your answer is "it would break backwards compatibility" then I understand, but I would also suggest (but not insist, I realize there are lots of good reasons not to break people's code) that the function could be changed over time with a deprecation schedule. If there are defenses of having params be an optional argument other than backwards compatibility then I would like to know those reasons.

Actually, a softer solution is to give a warning anytime the user doesn't pass in params that they're basically making a pretty bad move and should expect bad fit results unless they get lucky.

[newville]

@jagerber48 Well, for the gentle users who go to the trouble to read the documentation and examples, they will see that they can do this:

def gaussian(x, amp, cen, wid):
    """1-d gaussian: gaussian(x, amp, cen, wid)"""
    return (amp / (np.sqrt(2*np.pi) * wid)) * np.exp(-(x-cen)**2 / (2*wid**2))

gmodel = Model(gaussian)
result = gmodel.fit(y, x=x, amp=5, cen=5, wid=1)

print(result.fit_report())

I would like to not break that.

This is a fairly mature and stable project. We are mostly out of the design phase and focused on fixing problems. Features might be added as they are needed but with a strong emphasis on not breaking existing usage. When we do change things, we generally are solving documented problems.

You seem sort of committed to trying to change something. I think it would be necessary to identify a problem that you are having and want to solve. If that problem involves setting initial values for parameters, I think you will get only get a response of "the user needs to ensure that good initial values are provided" unless you can document a convincing use case that shows a real problem.

[jagerber48]

So there are two cases, one (1) where the users somehow provides initial values for all required parameters to seed the minimization algorithm and one (2) where the user omits one or more initial values for required parameters. Note that for the purposes of this discussion, I'm considering the required parameters to be the ones that appear in the function signature. I realize that when additional parameters are defined and constrained via algebraic relations the list of required parameters may differ from the list of parameters in the function signature. But to simplify things, let's just consider a model that has a function with N parameters in its signature and the user is trying to do a fit with N lmfit parameters that are 1:1 with those parameters in the function signature.

The former case (1) (where the user provides initial values for all N required parameters) I think you and I are both happy with. Especially after your initial comments, I gather that you want to encourage users to always be working in the former case. I don't deeply care if the initial values are passed in a formal Parameters object or as kwargs, though from a "zen of python" mindset, the kwargs approach feels a little implicit and like a second way to do things. But I see that for simple models the kwargs approach is a nice API that's pretty straightforwards.

But, in this thread, I'm investigating the behavior of the package in the LATTER case (2), in which the user has NOT provided sufficient initial value to seed the minimization routine. The package has two options in this case, (2A) it can either fail, and raise some kind of error, or (2B) it can try to patch the list of initial values so that it can proceed. My reading is that (2A) would be more in line with the spirit of "encouraging users to pass parameters".

But indeed, the package actually takes option (2B). If the user fails to pass in parameters, then the package patches the missing parameters with default values from the function. Taking options (2B) over (2A) is all about convenience because it allows the user to be more lazy. So upon seeing that the package takes option (2B), I wanted to suggest that it would be more convenient for the package to attempt to patch missing initial parameters with guess values rather than function default values, because, for generic data, the guess values have a much higher chance of resulting in a successful fit than the function default values. You shot that idea down as being too "magic" which I accept.

But I think that chain of reasoning leaves the package in a hypocritical state. You say that you don't want there to be magic under the hood that does stuff automatically, but the patching with default function parameters is exactly this. So in my eyes, the current behavior is the WORST of both worlds. The user can be lazy and pass nothing in, but what they'll get back is a really bad guess. The documentation warns against this, but I'm suggesting to put more protections in code. Either raising errors when users don't pass in enough initial parameters, or at least printing warnings.

You seem sort of committed to trying to change something. I think it would be necessary to identify a problem that you are having and want to solve. If that problem involves setting initial values for parameters, I think you will get only get a response of "the user needs to ensure that good initial values are provided" unless you can document a convincing use case that shows a real problem.

Consider this code:

import numpy as np
from lmfit.models import GaussianModel

model = GaussianModel()

model_x = np.linspace(1000, 1010, 100)
model_y = model.eval(x=model_x, amplitude=10, center=1005, sigma=1)

result = model.fit(model_y, x=model_x)
result.plot(show_init=True)

The fit totally fails:

It's obvious why the fit failed, it's initial parameter is 0 (which appears nowhere in the code block, but only deeply inside the lmfit.models modules) when the data has center = 1005. We know at the start of the fit that the fit is likely to fail because the user didn't provide initial parameters. Why not raise an error and force the user to provide initial values? I think this would be the best course of action. If this can't be done because it would break backwards compatibility (or some as-yet-unspecified other reason) then I would suggesting printing out a warning so that the failure isn't silent.

The code to detect if the users pass in all parameters would look something like taking the union of the parameters passed in in the params fit() function parameter and the additional parameters passed into fit() via kwargs and checking that those parameters are sufficient to seed the minimization algorithm. If they are not sufficient then either raise an error or print a warning and patch the missing parameters with function default values.

Your response could just be: "The fit failed because the user passed in bad guess parameters", no changes to the code are necessary. But what I'm suggesting is helping the user by telling them, in code, not just documentation, what they are doing wrong.

[newville]

@jagerber48 as we say REPEATEDLY, a fit that gives a bad result is not an error. Initial values always matter. Not 90% of the time, not 99.99% of the time.

Initial values matter for every single fit. The total number of exceptions to this rule over the lifetime of the universe is ZERO.

In addition, it is DEFINITELY NOT A BUG if a fit gives a bad result. Not something that needs fixing.

It's obvious why the fit failed, it's initial parameter is 0 (which appears nowhere in the code block, but only deeply inside the lmfit.models modules) when the data has center = 1005.

No, we do not. How would we know that at run time?

We know at the start of the fit that the fit is likely to fail because the user didn't provide initial parameters. Why not raise an error and force the user to provide initial values? I think this would be the best course of action. If this can't be done because it would break backwards compatibility (or some as-yet-unspecified other reason) then I would suggesting printing out a warning so that the failure isn't silent.

Um, read the fit report.

[jagerber48]

@newville It feels you're somehow missing my point.

@jagerber48 as we say REPEATEDLY, a fit that gives a bad result is not an error. Initial values always matter. Not 90% of the time, not 99.99% of the time.

Initial values matter for every single fit. The total number of exceptions to this rule over the lifetime of the universe is ZERO.

I obviously agree that "Initial values always matter" as you can see from all my text. That's why I'm giving suggestions for how lmfit can be more opinionated about forcing users to pass in good initial values. I'm suggesting that the current functionality where model.fit(y, x=x) succeeds to run without raising any warnings to the user encourages exactly the bad behavior that you are wanting to avoid.

In addition, it is DEFINITELY NOT A BUG if a fit gives a bad result. Not something that needs fixing.

Right, I'm not reporting a bug. At this point, I'm reporting a suggestion about how the UX could become more user friendly.

How would we know that at run time?

Here's a rough sketch. I try to cover the case where model parameters are more general than a 1:1 mapping to model function signature parameters using params.update_constraints() but I'd need to understand the code better or do some testing to see if I got it exactly right. There's probably some cases I don't cover, I threw this together quickly.

import inspect


def __init__(..., raise_missing_init_vals=False, ...)
    self.raise_missing_init_vals = raise_missing_init_vals


def fit(self, data, params=None, weights=None, method='leastsq',
        iter_cb=None, scale_covar=True, verbose=False, fit_kws=None,
        nan_policy=None, calc_covar=True, max_nfev=None, **kwargs):
    ...
    # Near the top of the fit() method:
    self.check_init_vals(params, **kwargs)
    ...


def check_init_vals(self, params, **kwargs):
    """
    Check that the user supplied params and kwargs include all the
    data necessary to seed the minimization algorithm. This means
    all independent variables must be included and, after updating
    parameter constraints, there must be initial values populated for
    all parameters corresponding to input parameters for the model
    function.
    """
    params = deepcopy(params)

    required_independent_vars = set(self.independent_vars)
    if not required_independent_vars.issubset(set(kwargs.keys())):
        raise ValueError(f'User supplied kwargs {kwargs} do not '
                         f'include values for all required'
                         f'independent variables '
                         f'{required_independent_vars}.')

    sig = inspect.signature(self.func)
    required_var_names = (set(sig.parameters.keys()) 
                          - required_independent_vars)
    user_supplied_var_names = set()

    for key, value in kwargs.items():
        if key in required_independent_vars:
            del kwargs[key]
        if isinstance(value, (float, int)):
            # The type check could be excluded, i.e. duck typing
            user_supplied_var_names.add(key)

    if params is not None:
        params.update(kwargs)
        params.update_constraints()
        for param_name, param in params.items():
            if param.value is not None:
                if isinstance(param.value, (float, int)):
                    # The type check could be excluded, i.e. duck typing
                    # The None check can not be excluded
                    user_supplied_var_names.add(param_name)

    if not required_var_names.issubset(user_supplied_var_names):
        if self.raise_missing_init_params:
            raise ValueError(f'The set of initial parameters supplied '
                             f'{user_supplied_var_names} does not '
                             f'include all parameters required for the '
                             f'fit {required_var_names}. Provide '
                             f'initial guess values for all required '
                             f'parameters.')
        else:
            warnings.warn( f'The set of initial parameters supplied '
                           f'{user_supplied_var_names} does not '
                           f'include all parameters required for the '
                           f'fit {required_var_names}. Missing initial '
                           f'parameters will be populated with default '
                           f'arguments from the model function. These '
                           f'arguments have no knowledge of the actual '
                           f'data so fit failure is likely. For better '
                           f'results, provide initial guess values for '
                           f'all required parameters.')

So the rough idea is to make sure the union of params and kwargs, after updating parameter constraints results in actual values supplied for all the input parameters to the model function. If this fails then either (depending on the raise_missing_init_params flag) raise an error and stop execution or print a warning and proceed with populating initial parameters with model function defaults.

The goal is that, as written (assuming I don't have some bug in there which would need to be tested out), this code wouldn't break any existing code. By default it may print warning for users who aren't passing in init values for all parameters. If users prefer a stricter approach they can set raise_missing_init_params=True to raise errors rather than print warnings.

Um, read the fit report.

Sure, I'd consider myself pretty experience at fitting, so reading the fit report can probably give me the information I need. I'm suggesting a feature that would be even more user-friendly towards people who might be less familiar with fit algorithms.

[jagerber48]

If this feature is out of scope for this project at it's level of maturity for some reason then that is fine I guess, though I don't fully understand the argument since it can be done in a backwards compatible way and is only the addition of a warning. It is a suggestion to make the tool more user-friendly. If you think it will not make the tool more user-friendly, or it will make it more user-friendly but it is not worth it anyways, then that is ok, we can leave it there.

[newville]

@jagerber48 I am simply unable to keep up with your changing suggestions. You seem intent to change something, quite possibly with the goal being to change something. I am not in favor of that.

You said earlier today that it "was obvious" why a fit failed when an initial value was "out of range". The code you propose does not try to catch that. Without knowing the details of the function itself, it is basically impossible to tell what a Parameter does, so it's not surprising that you don't try to catch this -- that would be very, very hard. But you did say it was obvious.

The person writing the function might have reasons to give default initial values to variables. For example, one reason might be to give "typical values", or so that keyword arguments can be clearly distinguished between "numerical arguments to be turned into parameters" vs "non-numeric arguments that are not to be turned into parameters".

That is, default values for arguments in a model function are definitely allowed because those are Python functions that allow them, and we are definitely going to continue to support code that relies on that advertised feature. That doesn't mean or imply in any way that those default values are suitable for all situations.

[jagerber48]

@jagerber48 I am simply unable to keep up with your changing suggestions.

You're right, there have been a few changes to what I'm suggesting, sorry about that. I'm always considering the case when users fail to supply initial values for all required parameters.

• First I was suggesting that these omissions should be patched with the model guess() function which you turned down and I accepted.
• Then I was suggesting that these omissions should NOT be patched with the default values that are coded into the model function signature definition. You seemed to not want to break existing code which I accepted. Though I still have the opinion that this "feature" of patching omitted initial values with model function default values is a bad feature that encourages bad behavior and likely leads to confusion for users.
• Now I'm suggesting that these omissions are detected and, by default, a warning is printed, but, optionally, an error can be raised.

You seem intent to change something, quite possibly with the goal being to change something. I am not in favor of that.

I'm not trying to change something just to change something. I'm trying to change to something to nudge inexperienced "fitters" (or fitters who want to be lazy) to scrutinize the initial parameters they pass (or fail to pass) to the fit algorithm.

You said earlier today that it "was obvious" why a fit failed when an initial value was "out of range". The code you propose does not try to catch that. Without knowing the details of the function itself, it is basically impossible to tell what a Parameter does, so it's not surprising that you don't try to catch this -- that would be very, very hard. But you did say it was obvious.

No, I don't want to try to catch when initial values are "out of range". That is similar to guessing initial parameters, and, as you say, would be hard. I am only trying to catch when users fail to provide initial values at all.

The person writing the function might have reasons to give default initial values to variables. For example, one reason might be to give "typical values", or so that keyword arguments can be clearly distinguished between "numerical arguments to be turned into parameters" vs "non-numeric arguments that are not to be turned into parameters".

That is, default values for arguments in a model function are definitely allowed because those are Python functions that allow them,

I'm not suggesting that model function should not HAVE default values for their function parameters. Those functions are just regular python functions and might be used outside the scope of lmfit for other purposes. I'm suggesting that relying on those default values for seeding the minimization algorithm should be deemed to be a bad practice, and this should be communicated to users in the form of an exception or warning.

and we are definitely going to continue to support code that relies on that advertised feature. That doesn't mean or imply in any way that those default values are suitable for all situations.

After some digging I found the documentation for this feature:
https://lmfit.github.io/lmfit-py/model.html#initializing-values-in-the-function-definition
I would suggest that this "feature" be documented a lot more prominently, including probably in the notes for model.make_params() (if it is used there, I can't quite tell) and especially model.fit()

My whole point now can be summarized by saying: I think this is a bad feature and, if, the feature will continue to be supported, as you say, I think users should be warned when they are relying on it.

Maybe this will add some context: For my use cases, I write a fit function once and then it is used for years across thousands or more datasets whose parameters vary by orders of magnitude, the idea of ever using the default values in the fit function just makes so little sense it's annoying to even have it as a possibility because it is a vector for me (or someone on my team) to make a mistake. I can see for very simple light weight use cases where the user is writing a fit function tailored to a specific data set (like when they're first learning about fitting or lmfit) where this feature might add a little bit of convenience, but for me, it's just not worth it.

To bring it back to my original suggestion (not because I think lmfit should go that way, just to make a point), the advantage to patching omitted initial values with results from the guess() function is that these patched values are aware of features of the data being fitted, unlike the model function default parameters. And I guess I'm confused (unless the answer is backwards compatibility/inertia), given your adamant insistence about the importance of initial values suited to the data set at hand, why you think automatic patching of omitted initial values with model function default values is acceptable while automatic patching with data-aware guess values is forbidden or somehow worse.

[jagerber48]

@newville sorry for any harsh/direct tones in the last message. I was getting a little frustrated since I felt you were missing or mischaracterizing my points. I feel like I make it sound like everything is terrible. In fact, it's the opposite. I think lmfit is an extraordinarily powerful and functional package for various kinds of fitting and analysis that I'm curious to adopt more heavily. There are just some very minor UI features like what I've been discussing here that I'm curious if (1) they can be changed if they're minor enough or (2) hear the justification for maintaining them the way they are.