lmfit-py icon indicating copy to clipboard operation
lmfit-py copied to clipboard

The `prefix` string is not added to parameter names for `CompositeModel`s

Open jcjaskula-aws opened this issue 10 months ago • 3 comments

Description

Strings passed via prefix when constructing a CompositeModel should be propagated to each parameter. Currently, it is missing for direct parameters (e.g. amplitude below) but properly used with derived parameters (like sigma).

A Minimal, Complete, and Verifiable example
from lmfit.models import GaussianModel, ConstantModel
from lmfit.model import CompositeModel
import operator
mm=CompositeModel(GaussianModel(prefix="G_"), ConstantModel(prefix="C_"), operator.add, prefix="prefix_")
mm.make_params()

returns image

mm.prefix returns the string prefix_.

I would expect prefix_G_amplitude, prefix_G_center, prefix_G_sigma, prefix_C_c and the derived parameters to have expressions using the just-mentioned parameters.

Version information

Python: 3.10.14 (main, Mar 19 2024, 21:46:16) [Clang 15.0.0 (clang-1500.3.9.4)]

lmfit: 1.3.1.post0+g3cfd09a1.d20240424, scipy: 1.11.4, numpy: 1.26.3,asteval: 0.9.32, uncertainties: 3.1.6

jcjaskula-aws avatar Apr 24 '24 12:04 jcjaskula-aws

@jcjaskula-aws I'm not sure I agree with your "should" here. I am not even certain that "prefix" makes sense with a Composite Model. You clearly said that the Gaussian Model should have a prefix of 'G_'.

The things that look to me like they need fixing are: a) that 'prefix_' got used as a prefix for the derived parameters, and b) how did 'prefix_G_sigma' get defined.

Disallowing 'prefix' with CompositeModel might be the most sensible fix.

(And, yes, no apologies for the delay in responding to this, since you chose to call it an Issue instead of a Discussion).

newville avatar May 11 '24 12:05 newville

My initial experience with this behavior was something like:

from lmfit.models import GaussianModel, ConstantModel
from lmfit.model import CompositeModel
import operator
mm=CompositeModel(GaussianModel(), ConstantModel(), operator.add, prefix="prefix_")
mm.make_params()

and I noticed that "prefix_" was not used at all in the parameters. As CompositeModel is a child of Model, I would expect it to have a prefix property.

I think it would improve the composability if prefix follows how Models are composed.

On the other side, I looked for a quick fix and I realized that it won't be straightforward so disallowing prefix is maybe a working solution to avoid unexpected behavior. FYI, this is not blocking me anymore. I now pass the prefixes directly to the subcomponents.

mm=CompositeModel(GaussianModel(prefix="prefix_"), ConstantModel(prefix="prefix_"), operator.add)

jcjaskula-aws avatar May 13 '24 16:05 jcjaskula-aws

@jcjaskula-aws To be clear, the expected usage would be not

mm=CompositeModel(GaussianModel(prefix="prefix_"), ConstantModel(prefix="prefix_"), operator.add)

but

mm = GaussianModel(prefix="peak_") + ConstantModel(prefix="offset_")

which seems a lot simpler to me, but doesn't allow for an additional prefix. CompositeModel() is needed for operators other than '+', '-', '*', and '/'. Of course, the vast majority of usage is with '+'.

FWIW, I do not think allowing a prefix for CompositeModel would be hard. It could insert itself before any prefix for the two Model components (left and right). But, I think this could be more confusing than helpful, especially as Composites work by chained binary composition, not full list composition.

Expecting the user to explicitly set all prefixes makes more sense to me than messing around with chaining prefixes.

newville avatar May 13 '24 19:05 newville