Model.eval with Parameters and kwargs can modify the Parameters

[mgunyho]

Hello,

I'm new to lmfit, and I noticed the following behavior: calling Model.eval with a Parameters object and kwargs can modify the Parameters. Here is a simple example that demonstrates this:

import numpy as np
import lmfit

def func(x, a, b):
    return a * x + b

model = lmfit.Model(func)
model.set_param_hint("a", value=3)
model.set_param_hint("b", value=2)


x = np.linspace(0, 10, 101)
y = 3 * x + 2 + np.random.default_rng().random(size=x.size) * 0.1

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

# here I would do e.g. fit_result.plot_fit(), omitted for brevity

# Suppose I want to compare what the fit would look like if I would set b = 0
y_with_b0 = model.eval(
    params=fit_result.params,
    b=0,
)

# here I would plot y_with_b0

print(fit_result.params["b"].value) # oops, b is now 0 in the fit result!

This is surprising to me. If the model is complicated and the fitting takes a long time, it's easy to accidentally overwrite my fit result. Is this intended behavior? Should I be doing something differently? A workaround I found is to use eval(params=fit_result.params.copy()), but that's easy to forget to do.

Model.eval calls Model.make_funcargs, where the kwargs are used to modify the Parameters object here. IMO this should not happen, I would expect that evaluating the model has no side effects. The parameters being modified is also not documented anywhere, only as a comment in the source of make_funcargs.

[mgunyho]

As a quick test, I removed the params[name].value = val line, and all tests pass. So maybe it is unintentional?

[newville]

@mgunyho

model = lmfit.Model(func)
model.set_param_hint("a", value=3)
model.set_param_hint("b", value=2)

Yuck. Is "a=3" a feature of the model? I doubt it.

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

Why did you not pass a set of parameters? Make a parameters object and use that. The normal usage would be

model = lmfit.Model(func)
params = model.make_params(a=3, b=2)
fit_result = model.fit(y, params, x=x)

Is this intended behavior?

Actually, I think it might be. You have

    y_with_b0 = model.eval(params=fit_result.params, b=0, x=x)

There is an ambiguity that you have introduced about whether to use the value of b in fit_result.params or in the explicitly passed value b. Updating params with the explicit values and then using that updated params seems reasonable.
It might seem sensible to make a copy inside of Model.eval, but eval is used internally during a fit, and I am pretty sure we do not want to be making such a copy each time we evaluate the function.

Should I be doing something differently?

Well, there are lots of things you could do. Making a copy is one option. Another option would be to make a parameters object and use that, perhaps something like:

   par_b0 = model.make_params(a=fit_result.params['a'].value, b=0)
   y_with_b0 = model.eval(params=par_b0, x=x)

[mgunyho]

Why did you not pass a set of parameters?

You are right, I forgot that. I am doing that in my actual code.

Is "a=3" a feature of the model?

In my actual code, I'm using set_param_hint(name, value=..., vary=..., min=..., max=...) (for several parameters) to specify a set of "default" parameters for the model (which are also used as an initial guess). Then I make several copies of the params (with make_params and/or Parameters.copy()) and modify e.g. vary or the initial guess or some other attribute for a subset of the parameters, and then I do fits for each set of parameters. That's why I'm using the set_param_hint here. Is there an easier way to say "do the same fit, but so that parameter x is held constant"? From what I recall when trying, doing model.fit(params=params, b=0) is only setting an initial guess, and it allows b to vary (I think even if I set vary=False in the param hint).

There is an ambiguity that you have introduced about whether to use the value of b in fit_result.params or in the explicitly passed value b.

The documentation explicitly says that kwargs override the params, and that is also how I would expect it to work. It makes sense to me that to quickly "temporarily" override some value, I would pass it as a kwarg, since modifying the Parameters object requires a bit more effort.

Updating params with the explicit values and then using that updated params seems reasonable. It might seem sensible to make a copy inside of Model.eval, ...

I would still say that it's a bit unexpected to have side effects in eval. Note that make_funcargs already creates a new dict where the values of the params are copied. It just sets the param values from the kwargs, but then does nothing with those. And it doesn't seem like the updated values of the parameters are used anywhere else during the fit afterwards, since the tests pass even if I remove the line where the parameters are modified.

[newville]

You are right, I forgot that. I am doing that in my actual code.

Unfortunately, we can only comment on the code you actually post.

Is "a=3" a feature of the model?
In my actual code, I'm using set_param_hint(name, value=..., vary=..., min=..., max=...) ...

So then, no "a=3" is not a feature of the model.

Model.set_param_hint is meant to (and does) set hints about how a model will generate initial parameters, it really ought to only contain hints intrinsic to the general model. For example, a Gaussian has a sigma parameter that represents a width - it makes no sense for this value to be negative. So the model can say that sigma has a minimum value of 0. The starting values and the range of allowed values for the other parameters depend on the data, not the model. So, parameter hints like amplitude=3 are inappropriate for the model.

Create parameters (which the code you posted notably avoids) and use those.

I would still say that it's a bit unexpected to have side effects in eval

Unexpected by whom? Does the documentation say that params will not be updated? If so, we should fix that. Because, during normal usage, one should definitely expect the values of parameters to be updated. It is kind of the point of the library.

Note that make_funcargs already creates a new dict where the values of the params are copied.

make_funcargs does not copy the Parameter object or the instance of the Parameters object. The language here might be somewhat confusing, but the distinctions are important. What is created by make_funcargs is a dictionary of {name: float_value} for each argument of that particular model function.

[mgunyho]

Unexpected by whom? Does the documentation say that params will not be updated?

Unexpected by me as a new user I suppose. The documentation doesn't say anything about the mutation one way or the other, I think the current behavior should definitely be documented if it is to stay.

during normal usage, one should definitely expect the values of parameters to be updated. It is kind of the point of the library.

Again to me, parameters changing when I evaluate the model after the fitting is not expected. This has bitten me several times, I've had to re-do fits because I forgot to .copy() when evaluating the model for plotting in an interactive session, for example. In general, I think side-effect-free functional-style code is easier to understand, which is also why I would prefer this mutation to be removed (and based on the tests, it wouldn't have an effect on the behavior of the rest of the library).

make_funcargs does not copy the Parameter object or the instance of the Parameters object

True, creating the dict is definitely more light-weight than doing a full .copy() of the Parameters.

[newville]

@mgunyho

Again to me, parameters changing when I evaluate the model after the fitting is not expected.

How would we know that you are calling Model.eval after a fit?

To me, doing

y = model.eval(params=my_params, b=0, x=x)

is really odd usage (to be clear, when my_params has a b Parameter). I might be more inclined to change the code to raise an exception if keyword arguments to Model.eval() clash with params. You are not calling the model function directly with a set of argument values, you are explicitly calling the eval method of the Model, and passing in a Parameters object. That implies (or, really, says explicitly) that you expect some aspects of the Model and of those Parameters to be used.

Since we do use Model.eval() internally in many places, so I would want to be very careful about changing it. If the params instance could not be updated, what values should constraint expressions (say, 'c=2*a+b') take?
Should bounds be applied to that value of b?

[mgunyho]

How would we know that you are calling Model.eval after a fit? ... model.eval(params=my_params, b=0, x=x) is really odd usage

I would assume that this is one of the main points of the library: fit a model curve to some data, extract the model parameters, and then make predictions using the model. Maybe my use case of "evaluate the model, but with this parameter set to some other value" is slightly unusual. But it's something that I do constantly during exploratory data analysis, when trying to figure out if the model I'm using is even right for the data, or if my model has a bug.

raise an exception if keyword arguments to Model.eval() clash with params

I think that would be safer than the current option (so that you don't accidentally overwrite the fit result), but it would make exploring the result way more inconvenient. Instead of just model.eval(params=fit_result.params, b=0, x=...), I would have to write

p_copy = fit_result.params.copy()
p_copy["b"].value = 0
model.eval(p_copy, x=...)

which is cumbersome and creates unnecessary temporary variables.

That implies (or, really, says explicitly) that you expect some aspects of the Model and of those Parameters to be used.

You are right. To me it would feel natural that the passed in params would override the attributes of the model, and that the kwargs would in turn override the attributes of the params. This is going from "most stable" to "least stable" in the sense that the model is modified rarely (basically only when creating it), the params is modified ~once per fit (from the user's POV) but possibly several times for the same model, and the kwargs would be passed for ephemeral quick evaluations of the model function.

we do use Model.eval() internally in many places, so I would want to be very careful about changing it

I agree that it is a bit scary to modify, and this should be announced as a breaking change. Still, as I said, all tests pass without the mutation, so this case has apparently not been considered much.

what values should constraint expressions (say, c=2*a+b') take?

I would say that they should be evaluated with the kwargs version of b. Basically I would expect the behavior to be roughly along the lines of

def eval(self, params, **kwargs):
    param_values = {
        k: v.value for k, v in self.make_params().items()
        if v.value is not None
    }

    param_values.update(params.valuesdict())

    for k, v in kwargs.items():
        if params[k].expr is not None:
            # Here, we would evaluate the Parameters._asteval
            # with param_values as the symbol table.
            param_values[k] = evaluate_expr(...)
        else:
            param_values[k] = v

    return self.func(**param_values)

Of course this is glossing over lots of details (like the prefix stuff I see in make_funcargs). It also seems like this isn't easy to do: apparently asteval.Interpreter is also stateful, so it can't be used to evaluate expressions with a different symbol table without making a copy of the whole interpreter.

Should bounds be applied to that value of b?

Good question, I'm not entirely sure. It could be useful to explore how the model behaves when going outside the boundaries, so I think it should at most issue a warning. Looks like the current behavior with bounds is also a bit weird, when evaluating the model, the bound is ignored, but the parameter value is left at the boundary:

import lmfit

def func(x, a, b):
    return a * x + b

model = lmfit.Model(func)
model.set_param_hint("b", min=0)
p = model.make_params(a=3, b=2)

print(model.eval(params=p, b=-100, x=1))  # prints -97
print(p["b"].value)  # prints 0

I looked at the documentation a bit more carefully, and it briefly mentions that initial values for the parameters can be supplied with eval(), although there is no discussion of combining params and kwargs. (The docs also talk about using set_param_hint as one of the options for setting initial values, so maybe that's where I got that idea from.)

[newville]

On Tue, Feb 14, 2023 at 4:33 AM mgunyho ***@***.***> wrote: How would we know that you are calling Model.eval after a fit? ... model.eval(params=my_params, b=0, x=x) is really odd usage I would assume that this is one of the main points of the library: fit a model curve to some data, extract the model parameters, and then make predictions using the model.

Yep. But we still do not know when you call `Model.eval()` whether that is before, during, or after a fit, or maybe "between fits".

Maybe my use case of "evaluate the model, but with this parameter set to some other value" is slightly unusual.

It's not. But your usage of mixing a Parameters instance and individual values in `Model.eval()` probably is unusual. At least, I hope it is unusual. That ambiguously supplies two values for the same parameter, and we have to decide what to do with that ambiguity. I would say the ambiguity in your usage is *intentional*.

But it's something that I do constantly during exploratory data analysis, when trying to figure out if the model I'm using is even right for the data, or if my model has a bug. raise an exception if keyword arguments to Model.eval() clash with params I think that would be safer than the current option (so that you don't accidentally overwrite the fit result), but it would make exploring the result way more inconvenient. Instead of just model.eval(params=fit_result.params, b=0, x=...), I would have to write p_copy = fit_result.params.copy()p_copy["b"].value = 0model.eval(p_copy, x=...)

Yep, creating, modifying, copying, and updating Parameters objects seems like a perfectly normal thing to do. Parameters objects are kind of the key component of lmfit. In addition to changing the value, you could also change bounds, expressions, and whether certain parameters are fixed. This is very useful for exploratory data analysis, as you can redo fits with different parameter values and settings. which is cumbersome and creates unnecessary temporary variables.

I think it is actually what you want.

That implies (or, really, says explicitly) that you expect some aspects of the Model and of those Parameters to be used. You are right. To me it would feel natural that the passed in params would override the attributes of the model, and that the kwargs would in turn override the attributes of the params.

Hmm, I think that "override the attributes of the params" is what we are currently doing. This is going from "most stable" to "least stable" in the sense that the

model is modified rarely (basically only when creating it),the params is modified ~once per fit (from the user's POV) but possibly several times for the same model, and the kwargs would be passed for ephemeral quick evaluations of the model function.

Parameter values may be altered once per step/iteration of the fit -- specifically **for each evaluation of the fitting function**. That is, for each call of `Model.eval()`. we do use Model.eval() internally in many places, so I would want to be

very careful about changing it I agree that it is a bit scary to modify, and this should be announced as a breaking change. Still, as I said, all tests pass without the mutation, so this case has apparently not been considered much. what values should constraint expressions (say, c=2*a+b') take? I would say that they should be evaluated with the kwargs version of b. Basically I would expect the behavior to be roughly along the lines of def eval(self, params, **kwargs): param_values = { k: v.value for k, v in self.make_params().items() if v.value is not None } param_values.update(params.valuesdict()) for k, v in kwargs.items(): if params[k].expr is not None: # Here, we would evaluate the Parameters._asteval # with param_values as the symbol table. param_values[k] = evaluate_expr(...) else: param_values[k] = v return self.func(**param_values) Of course this is glossing over lots of details (like the prefix stuff I see in make_funcargs). It also seems like this isn't easy to do: apparently asteval.Interpreter is also stateful, so it can't be used to evaluate expressions with a different symbol table without making a copy of the whole interpreter.

What we would actually need to do is make a copy of the parameters and update the values with your passed-in-by argument values. That would "create a temporary set of Parameters", but one that the user would never see. We do not want to do that at every function evaluation during a fit. Minimize() (and so also Model()) does, in fact, make a copy of the input Parameters at the start of each fit, and then updates that working copy during the fit. That is once per fit, not once per function call. Should bounds be applied to that value of b?

Good question, I'm not entirely sure.

Using a Parameters instance would give a clear answer (Yes, bounds should be obeyed). If you want to change the boundaries, you can do that too. It could be useful to explore how the model behaves when going outside the

boundaries, so I think it should at most issue a warning. Looks like the current behavior with bounds is also a bit weird, when evaluating the model, the bound is ignored, but the parameter value is left at the boundary: import lmfit def func(x, a, b): return a * x + b model = lmfit.Model(func)model.set_param_hint("b", min=0)p = model.make_params(a=3, b=2) print(model.eval(params=p, b=-100, x=1)) # prints -97print(p["b"].value) # prints 0 I looked at the documentation a bit more carefully, and it briefly mentions <https://lmfit.github.io/lmfit-py/model.html#initializing-model-parameters> that initial values for the parameters can be supplied with eval(), although there is no discussion of combining params and kwargs.

I view this as more evidence that using an actual Parameters instance is the better option. (The docs also talk about using set_param_hint as one of the options for

setting initial values, so maybe that's where I got that idea from.)

Yeah, I think `set_param_hints` should be more-or-less a protected method, used within subclasses, but not part of the public API.

[mgunyho]

But we still do not know when you call Model.eval() whether that is before, during, or after a fit, or maybe "between fits".

Ah, I see what you mean now. But I think this is also an argument in favor of not modifying state (i.e. the passed in Parameters object) during eval(). If eval() would be a pure function, it wouldn't need to know or care whether we are in the middle of fitting or not. The caller should know that. For example, updating the parameter values is the job of the fit function.

Hmm, I think that "override the attributes of the params" is what we are currently doing.

By "override" I mean "temporarily override for the duration of this function call". This is something I want to do with very low friction when fiddling with the model/fit result. Making the copy and so on is a lot of extra typing. IMO, the wording of the documentation for eval also suggests this:

If params is given, and a keyword argument for a parameter value is also given, the keyword argument will be used.

I think the main confusion is that there are two ways to pass the parameter values to eval, and they are apparently intended to be mutually exclusive. All values should be either passed via params, or via kwargs (in addition to the free parameter(s) of course). When I saw the API, I assumed that the point of the kwargs is to temporarily override parameter values. This would be very convenient, but apparently it's not the intended use?

In a way, the current API almost supports this use case (passing params=params.copy(), a=... is still much more convenient than writing p_copy = params.copy(); p_copy["a"] = ...; eval(p_copy)), and based on the tests, it would be possible to support it. So I would prefer to have this behavior changed.

[Tillsten]

Just a as datapoint as a small time contributer, even if it is not the recommend way, i often use set_param_hint the same way as @mgunyho. In many simple cases, I do not need the explicted parameters as their own object, e.g.

mod = lmfit.models.GaussianModel()
mod.set_param_hint("amplitude", min=0, value=3)
fit = mod.fit(y, x=x, sigma=3, center=2000)

In general, I save a reference the fit results object anyways, hence having an explict reference to the parameters afterwards is often not that helpful. Also models are rather cheap, so making a new one is not that bad outside of tight loops.

[newville]

@Tillsten Hm, I am continually baffled by people using Model.set_param_hint. I'm sure it is me ;).

It must be that "creating parameters with default values and bounds" is too difficult. Maybe we should create a convenience function like

params = create_params(amplitude={'value':3, 'min':0}, 
                                            center=300, 
                                            sigma={'value':3, 'min':0}, 
                                             fwhm={'expr':'2*sigma'}
                                             prefix='a')

that would
a) create params from argument names.
b) if the value is a simple float, that's the value.
c) if the value is a dict, that is used to set up the parameter, with standard keywords (value, min, max, expr, vary, brute_step,...)

and/or maybe we should update Model.make_params() to use accept value or dict in that way.

There could also be a Parameters.update() method that uses the same interface, so that, as here, one could do

 pars = fit_result.params.copy()
 pars.update(b=7, a={'vary':False})
 ## proceed to redo the fit with the updated parameters.

@mgunyho @reneeotten Any thoughts on this?

[mgunyho]

and/or maybe we should update Model.make_params() to use accept value or dict in that way.

This is something that I thought of as well. I think it would be very nice to be able to do make_params(a=1.0, b=dict(min=0, max=10)), and I think I would stop using set_param_hint if that were possible :)

[reneeotten]

@newville I'll have to take a look at this thread a bit better. Will try to do so sometime this week and respond here.

[mgunyho]

Another option here could be to have a copy_with method, like params.copy_with(b=0, a={'vary': False}) (could also be called updated, although copy_with is clearer IMO). This could make it less necessary to create temporary variables.

[newville]

@mgunyho I very much doubt that we are not going to alter Model.eval() to create a temporary Parameters object to handle the case of clashes between parameters named in a params object and the method argument list.

As I am sure you know, Python does not use "pure functions". Values are passed into methods by reference and may be updated by the method. Calling an object's method (here "evaluate the Model with Model.eval()") may very well change the state of the instance -- that would be completely normal Python behavior. Note that you are calling Model.eval(), you are not calling func(). That is sort of saying that you want more than just calling func() to happen, right?

But, it turns out that Model.eval() does not change the Model's state (it does not cause any Model attributes to be changed). The parameters argument passed in is not part of the state of the Model, but is literally input to the model, as data arrays would be. And mutable objects passed to a method might be altered by the method.

Your usage of Model.eval() is ambiguous (and kind of deliberately so). You could have done

test_result = model.eval(a=3, b=7, x=np.array([1,2,3,4,5]))

or

params = model.make_params(a=3, b=7)
test_result = model.eval(params, x=np.linspace(-5, 5, 21))

Both of those are completely acceptable and clear. But what you have done is

params = model.make_params(a=3, b=7)
test_result = model.eval(params, b=9000, x=np.linspace(-5, 5, 21))

That is ambiguous. We do not promise in the API or the documentation what we do in such usage. The fact that we do not already raise an exception is what surprises me.

It is perfectly reasonable to expect that a model will not normally update its input parameters, but we do not promise that this will never happen. For your ambiguous usage, "normal" may not apply.

I write apps with lmfit to allow users to do exploratory fitting, change values, evaluate models with updated values, change bounds and constraints, compare fits, view fit histories, and so forth. I'm not saying that this could not be smoother (see earlier suggestion), but I do not really see making copies of parameters as "cumbersome" or "inconvenient".

[mgunyho]

I very much doubt that we are not going to alter Model.eval() to create a temporary Parameters object

What I'm proposing is to remove these two lines:

lmfit-py/lmfit/model.py

Lines 828 to 829 in 729491d

	if name in params:
	params[name].value = val

This would make eval() work as I expect.

That is ambiguous. We do not promise in the API or the documentation what we do in such usage.

The documentation says:

If params is given, and a keyword argument for a parameter value is also given, the keyword argument will be used.

So I would say it's not ambiguous what is done in this case. Perhaps "use" could be interpreted as "update params with the values from kwargs, and then use the updated params", but I don't think most people would understand it that way.

Values are passed into methods by reference and may be updated by the method.

Sure, Python allows mutating any objects that you pass as arguments to a function. But I think side effects should be avoided as much as possible, since it makes code harder to reason about (this is the main selling point of functional programming). As a rule of thumb, I would expect a method to only mutate self, unless it's very clear that its purpose is to modify another object.

[newville]

I think raising an exception at

lmfit-py/lmfit/model.py

Line 829 in 729491d

params[name].value = val

is the correct behavior.

The documentation says:
If params is given, and a keyword argument for a parameter value is also given, the keyword argument will be used.

Yep, we should change that to raising an exception, to avoid guessing the users expectation about whether the parameters are updated or not.

Sure, Python allows mutating any objects that you pass as arguments to a function. But I think side effects should be avoided as much as possible,

Yep, that is what we do.

[mgunyho]

But do you see a downside to enabling this use case, instead of forbidding it by raising an exception? Assuming of course that nothing indeed breaks, as the tests would indicate.

[newville]

@mgunyho Yes, I do see a downside in supplying two values for b, one in params and one from the function keyword arguments. That is confusing.

Either the inputs should be made consistent (which you object to) or the outputs are ambiguous. The choices I see are
a) leave as is. You object to the current behavior, though it does ensure consistency of input and output.
b) don't update params but do the calculation, preferring keywords over parameter values, updated in the background without clarity for how bounds or constraints are handled, leaving input and output inconsistent.
c) raise an exception.

I think b) is the worst of these -- it's not clear what to do with bounds or constraint expressions (either on that parameter itself, or using that parameter). If we're looking for consistency and fewer surprises, then c) seems like the best choice.

"In the face of ambiguity, refuse the temptation to guess".

[mgunyho]

When you talk about ambiguity, are you specifically referring to expression evaluation and boundary checks? Is that what you mean by consistency? Would you agree that eval(params, b=0) if there are no expressions depending on b, and it is within bounds, along with the docstring "if a parameter appears both in params and kwargs, use the value from the kwargs" is unambiguous? Because this is what I had in mind. Maybe we have misunderstood each other here.

I agree that if the kwargs violate the constraints/expressions of the params, it is not obvious what should happen. Raising an exception in that case could be reasonable, but it might also be useful to be able to explore what the model produces even if the constraits are violated. Of course in practice it's not trivial to check whether any of the expressions need to be re-evaluated.

Just to clarify, if option c) would be implemented, would the recommended way to "evaluate the model with a from the fit result but with b set to 0" be to do p = fit_result.params.copy(); p["b"].value = 0; model.eval(p)? Or is there a more concise way to do it? Basically I'm looking for the easiest way to do this.

[newville]

On Thu, Feb 16, 2023, 1:12 AM mgunyho ***@***.***> wrote: When you talk about ambiguity, are you specifically referring to expression evaluation and boundary checks? Is that what you mean by consistency? Would you agree that eval(params, b=0) *if there are no expressions depending on b, and it is within bounds*, along with the docstring "if a parameter appears both in params and kwargs, use the value from the kwargs" is unambiguous? Because this is what I had in mind. Maybe we have misunderstood each other here.

If `params` has a `b`, then there is an ambiguity of which value to use. That is the ambiguity. If we do not update `params`, then we are left with potential problems of consistency. We have worked out that `Parameters` do return consistent values, and we are not going to hack parts of that into `Model` when it creates values for function arguments. I agree that if the kwargs violate the constraints/expressions of the

params, it is not obvious what should happen. Raising an exception in that case could be reasonable, but it might also be useful to be able to explore what the model produces even if the constraits are violated. Of course in practice it's not trivial to check whether any of the expressions need to be re-evaluated.

If the user wants a constraint or bounds violated they need to lift those. Just to clarify, if option c) would be implemented, would the recommended

way to "evaluate the model with a from the fit result but with b set to 0" be to do p = fit_result.params.copy(); p["b"].value = 0; model.eval(p)? Or is there a more concise way to do it? Basically I'm looking for the easiest way to do this.

Yep, that would be the recommendation. Claims that it is hard are just not going to persuade me. Again, you could call the model function with values directly. You can also call Model.eval with values only. It is when you mix Parameters and individual values that should be avoided, and should raise an exception.

[newville]

@mgunyho but I will also say that as I look into this more, it does seem that your suggestion to simply not update the value in params could work. As you say, all the existing tests do pass. I think it might be workable to update, get the latest values, and then reset them... investigating...

[mgunyho]

Sorry for the delay, and also for dragging this out so much.

If params has a b, then there is an ambiguity of which value to use. That is the ambiguity.

In my opinion, with the wording of the documentation, it's not ambiguous at all: "If a parameter is both in params and kwargs, the value from kwargs will be used."

If we do not update params, then we are left with potential problems of consistency. We have worked out that Parameters do return consistent values, and we are not going to hack parts of that into Model when it creates values for function arguments.

I agree that checking the consistency is certainly the job of Parameters and not Model. I understand that your concern is that if kwargs contradicts the constraints in params, the model will be evaluated with a set of parameters that are inconsistent with the constraints, is this correct? This is what you mean by consistency, right?

That is fair, and I think it's a reasonable option to throw an exception if kwargs contradicts the constraints (an alternative would be to just issue a warning, in case the user wants to explore what would happen if the constraints are violated). But an exception shouldn't be raised if there is no contradiction.

I would reframe option b) like this: Don't modify (/update) params, but do the calculation. It's possible that the model is evaluated with a set of parameters that are inconsistent with each other / don't satisfy the constraints, just like it has been possible to do already. However, there is no risk of the user overwriting their fit result.

Note that with option a) the input and output is not guaranteed to be consistent. In the example I gave (#839 (comment)) with params["b"].min = 0 and model.eval(params, b=-100), the function is evaluated without considering the bounds, just that after the evaluation, b will have a value that is consistent with them. But if the parameters were never modified, this would also be true, just that b would have its original value (which we assume satisfies the constraints), and I would argue that this is less confusing.

A reasonable "fix" would be to just explicitly state in the documentation that if both params and kwargs are given, constraints are not checked and it's the responsibility of the caller to ensure they are upheld.

Claims that it is hard are just not going to persuade me.

It's not hard, it's just very inconvenient. Writing p = fit_result.params.copy()<enter>p["b"].value = 0<enter>model.eval(p) is a lot of extra typing compared to model.eval(fit_result.params, b=0). Similarly for e.g. model.eval(**dict(fit_result.params.valuesdict(), b=0)). It just creates unnecessary friction. I think this would discourage exploration, especially for people who are not fast typists. Also, my Jupyter notebooks already have too many single-letter temporary variables scattered around, so I would like to create as few of them as possible.

[newville]

@mgunyho OK, there definitely was a severe problem with supporting mixed parameters and keywords. A perfectly valid model (and one much like I really must work), would be:

from lmfit.models import GaussianModel
model = GaussianModel(prefix='g1_') + GaussianModel(prefix='g2_')
params = model.make_params(g1_amplitude=8, g1_center=8, g1_sigma=1,
                           g2_amplitude=8, g2_center=15, g2_sigma=1)
params['g2_sigma'].expr = 'g1_sigma'

You have a composite model, with parameters from one model used to constrain parameters in another model. The problem is that

model.eval(params, g1_sigma=2, x=10)

currently fails: the value of g2_sigma is not updated too. I think I have a fix that will allow this and also your request. To fix the constraint problem the parameters object does need to be updated and the constraints correctly evaluated. But then once the values are extracted, the parameter values can be restored before returning.

If params and keywords are mixed, this will be a slight performance hit to give correct results. But fitting always updates params directly, so it should not impact fitting performance.

[mgunyho]

You are right, this is also a good example of something unexpected happening. There should be a test to check the behavior for a case like this, whatever we expect it to be. Looks like none of the tests actually call model.make_funcargs on its own, and eval seems to be also called only with either params or kwargs.

the parameters object does need to be updated and the constraints correctly evaluated. But then once the values are extracted, the parameter values can be restored before returning

I really think updating is not what we want to do. Messing with the state and then restoring it is error prone - for example, what if you get an exception from the func? Yes, it can be dealt with, but it makes things complicated and the code messy and hard to follow.

Ideally, it should be possible to give a Parameters object a set of values and constraints/expressions for unevaluated parameters, and ask it to return the set of evaluated values that are consistent with the constraints (or raise an exception if there is an inconsistency). Checking constraints and evaluating a set of expressions should really not have any state, it should be a pure mapping from values + expressions to values, right?

But I understand that this is not easy (or maybe even possible) to do with the current architecture, since the internal asteval object seems to be also based around managing state and checking for consistency when a parameter is set, and not at evaluation time.

I'm sure that you can make it work this way, I'm just worried that it will lead to more obscure bugs down the road.

[newville]

@mgunyho Sorry, trying to helpful and positive, but also confident that I do have some understanding of this code ;)

If params has a b, then there is an ambiguity of which value to use. That is the ambiguity.

In my opinion, with the wording of the documentation, it's not ambiguous at all: "If a parameter is both in params and kwargs, the value from kwargs will be used."

The ambiguity is in the code. It permits the user to supply two values for a parameter. The code has to decide what to do in the face of this ambiguity. The ambiguity exists.

The documentation may describe how such ambiguity is handled. The documentation does not remove the ambiguity. The documentation might be wrong. The documentation might be changed.

... the model will be evaluated with a set of parameters that are inconsistent with the constraints, is this correct? This is what you mean by consistency, right?

Yes. If the set of parameter values do not obey the constraints, they are wrong. That is a very serious error.

That is fair, and I think it's a reasonable option to throw an exception if kwargs contradicts the constraints (an alternative would be to just issue a warning, in case the user wants to explore what would happen if the constraints are violated). But an exception shouldn't be raised if there is no contradiction.

Nope. In the example I posted, g1_sigma=2.2 does not contradict the constraint. But the value of g2_sigma needs to follow that.

Note that with option a) the input and output is not guaranteed to be consistent. In the example I gave (#839 (comment)) with params["b"].min = 0 and model.eval(params, b=-100), the function is evaluated without considering the bounds, just that after the evaluation, b will have a value that is consistent with them. But if the parameters were never modified, this would also be true, just that b would have its original value (which we assume satisfies the constraints), and I would argue that this is less confusing.

The function must be called with values that consider any bounds and constraints. Violating bounds is a serious error and cannot be allowed.

A reasonable "fix" would be to just explicitly state in the documentation that if both params and kwargs are given, constraints are not checked and it's the responsibility of the caller to ensure they are upheld.

No.

Claims that it is hard are just not going to persuade me.

It's not hard, it's just very inconvenient. Writing p = fit_result.params.copy()p["b"].value = 0model.eval(p) is a lot of extra typing compared to model.eval(fit_result.params, b=0). Similarly for e.g. model.eval(**dict(fit_result.params.valuesdict(), b=0)). It just creates unnecessary friction. I think this would discourage exploration, especially for people who are not fast typists. Also, my Jupyter notebooks already have too many single-letter temporary variables scattered around, so I would like to create as few of them as possible.

Sorry, I am not even slightly persuaded here.

You are right, this is also a good example of something unexpected happening. There should be a test to check the behavior for a case like this, whatever we expect it to be. Looks like none of the tests actually call model.make_funcargs on its own, and eval seems to be also called only with either params or kwargs.

That's OK. Model.make_funcargs is only used by Model.eval(). That is the only context in which it needs to work. it is not even called by subclasses like CompositeModel, which is actually part of the point. This is illustrated in the 2-Gaussians example I gave.
Model.make_funcargs will be called twice -- once to turn the parameters named g1_amplitude, g1_center, g1_sigma into values for amplitude, center, and sigma to send to the gaussian() function, and once to turn the parameters named g2_amplitude, g2_center, g2_sigma into those values. In the g2 case it has to get the right value, which is only guaranteed to be correct by updating all parameter values with any values passed in as keywords (even though a naive lexical analysis might conclude "oh, it starts with g1, we don't need it") and updating the constraint values (which also applies bounds). This has to be done or the values are not correct.

This fails in current master branch. The good news is that I have a fix for it. If we did not have a fix for it, we would deprecate mixing keywords and parameters.

the parameters object does need to be updated and the constraints correctly evaluated. But then once the values are extracted, the parameter values can be restored before returning

I really think updating is not what we want to do.

It is definitely what we want to do.

Messing with the state and then restoring it is error prone - for example, what if you get an exception from the func? Yes, it can be dealt with, but it makes things complicated and the code messy and hard to follow.

Well, make_funcargs extracts the appropriate parameter values -- and would restore any changed values -- before the users model function is called. Changing Parameter values and updating constraints is very common - it happens for every function evaluation in a fit. Also doing it if a user calls Model.eval mixing parameters and keywords is not going to introduce new problems.

Ideally, it should be possible to give a Parameters object a set of values and constraints/expressions for unevaluated parameters, and ask it to return the set of evaluated values that are consistent with the constraints (or raise an exception if there is an inconsistency). Checking constraints and evaluating a set of expressions should really not have any state, it should be a pure mapping from values + expressions to values, right?

No. Checking constraints and evaluating a set of expressions does and must have state. The expression itself is state.

But I understand that this is not easy (or maybe even possible) to do with the current architecture, since the internal asteval object seems to be also based around managing state and checking for consistency when a parameter is set, and not at evaluation time.

With

params = Parameters()
params.add('a', value=2, min=0)
params.add('b', value=10)
params.add('c', expr='2*b - a')

then return the value of c depends on the state of the expression, and the values for a and b. Technically, it also depends on the state of what '2', '*', and '-', which is more or less what "the expression is evaluated with asteval" gives. That might seem silly or trivial, but if the expression was '2*sin(b)-a', that state should become more obviously non-trivial. ;)