Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The prefix string is not added to parameter names for CompositeModels #954

Open
jcjaskula-aws opened this issue Apr 24, 2024 · 3 comments
Open

Comments

@jcjaskula-aws
Copy link
Contributor

jcjaskula-aws commented Apr 24, 2024

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 jcjaskula-aws changed the title prefix string is not added parameter names for CompositeModels The prefix string is not added to parameter names for CompositeModels Apr 24, 2024
@newville
Copy link
Member

@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).

@jcjaskula-aws
Copy link
Contributor Author

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)

@newville
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants