symfit icon indicating copy to clipboard operation
symfit copied to clipboard

Added metaclass to create dummy Paramter/Variables

Open Jhsmit opened this issue 4 years ago • 4 comments

This is a bit of a hack, but it allows users to force creation of dummy Parameter / Variables instead of symbol ones (see #290).

Usage:

import builtins
builtins.symfit_dummy = True

from symfit import Parameter

a = Parameter('x', value=2)
b = Parameter('x', value=3)

print(a.value, b.value)


>>> (2, 3)

Where in normal symfit behaviour the result would be (3, 3).

Why would you want to use this? Well, if your application of symfit is one script, one model kind of fitting then there is probably very little use. But if you make a bigger application where many models are created and active at the same time (thread safe?) then dummy behaviour is desired.

Also, lets say you happen to be making your parmeters like this:

A = Parameter()
sig = Parameter('par_0')

Without dummies, these two parameters are the same, and fitting fails without an error. With dummies, it still doesnt work, but at least you get an error :)

Yes, I agree the implementation is a bit sketchy and could be considered dark magic. Suggestions are welcome. Maybe some split of the inheritance into Parameter and DummyParameter, but because of all the implementations of __new__ it seems that Dummy somehow needs to be introduced just after Argument in the mro. The hack via builtins is needed because the information on wheter to dummy or not needs to be passed before any symfit imports as that is when the classes are created. builtins is py3 only (did we drop py2 already?)

Jhsmit avatar Feb 07 '20 18:02 Jhsmit

I like the idea for creating this, but I would rather choose a solution that stays closer to sympy to prevent problems in the future. I think the following object structure should work:

BaseParameter - defines slots min, max, fixed, value. This is a mixin class and doesn't inherit from anything. Parameter(BaseParameter, Argument) DummyParameter(BaseParameter, sympy.Dummy)

I think this should work, what about you?

tBuLi avatar Mar 19 '20 14:03 tBuLi

Yes I agree this PR should not be merged under any circumstances. But for it a was a fun trip into the madness that is symfit, sympy and metaclasses.

The structure you proposed is indeed the best solution in my opinion. I dont remember the details but I had trouble envisioning it because of I ran into some MRO issues. But as a mixin it might work.

We'd also need to do the same thing for Variables then but that should be much easier.

Jhsmit avatar Mar 19 '20 14:03 Jhsmit

Yeah I had those same problems when trying to include indexed variables and parameters and back then I eventually found that a mixin was the only way of getting it to work if you didn't want to go down a very dark path.

I also don't think the solution above should be very hard to implement, and I would prefer having DummyParameters and DummyVariables as seperate classes so we don't create unexpected problems. And we can add a dummy keyword to the parameters and variables support functions for convenience.

tBuLi avatar Mar 19 '20 14:03 tBuLi

Perhaps Parameters, Variables should always be Dummy's in a future (major) version of symfit? I don't see a clear use case for the non-dummy behavior.

Perhaps you could consider someone wanting to do this:

model = {
Variable('y1'): Parameter('a')*Variable('x') + Parameter('b'),
Variable('y2'): Parameter('a')*Variable('x')**2
}

Which with dummy variables you would have to define by:

y1_var = Variable('y1')
y2_var = Variable('y2')
a_par = Parameter('a')
b_par = Parameter('b')

model = { ...

Where I do agree that the former has a nicer syntax and the latter pollutes the local scope but this example ignores the fact that on parameters you have to define initial guesses so usually you cannot write your problem as option 1 anyway.

EDIT: forgot to mention here that if I naively make IndexedParameter/Variables for my own internal use from sympy they appear to be the dummy variation. Therefore my thinking that everything should be dummy.

Jhsmit avatar Mar 06 '22 13:03 Jhsmit