tBuLi / symfit

Symbolic Fitting; fitting as it should be.
http://symfit.readthedocs.org
MIT License
233 stars 17 forks source link

Allow declaration of Dummy arguments instead of just Symbols #290

Open JohnGoertz opened 4 years ago

JohnGoertz commented 4 years ago

I'm not sure if this would break everything, but it would be very useful to be able to construct parameters/variables that inherit sympy's Dummy class instead of the Symbol class. The reason being that if multiple variables are created with the same string input, they are the same object and changing one changes the other.

import symfit as sf
symbols = 'x,y'
a = sf.parameters(symbols)
b = sf.parameters(symbols)
print(a is b)
print(a[1] is b[1])
print(b[1].value)
a[1].value = 2
print(b[1].value)
False
True
1.0
2

This is even true if these variables are containerized into class instances.

class MyContainer:
    def __init__(self,symbols):
        self.params = sf.parameters(symbols)

a = MyContainer(symbols)
b = MyContainer(symbols)
print(a is b)
print(a.params[1] is b.params[1])
print(b.params[1].value)
a.params[1].value = 2
print(b.params[1].value)
False
True
1.0
2

Sympy solves this through the Dummy variable class, so if there was an alternative constructor for symfit's Argument class to extend Dummy, that (should) fix the issue.

import sympy
a = sympy.symbols(symbols)
b = sympy.symbols(symbols)
print(a is b)
print(a[1] is b[1])
a = sympy.symbols(symbols, cls=sympy.Dummy)
b = sympy.symbols(symbols, cls=sympy.Dummy)
print(a is b)
print(a[1] is b[1])
False
True
False
False

Now, I'm not sure if this is more properly a sympy bug or if this is actually desirable behavior, or if it would be a nightmare to implement, but... it would be nice.

pckroon commented 4 years ago

I think that it's desired behaviour, as far as sympy is concerned. The good news is that we subclass Symbol fairly transparently: could you try b = symfit.Parameter(name='b', cls=sympy.Dummy) and see where it breaks?

Jhsmit commented 4 years ago

This was also discussed a while ago in #124

Jhsmit commented 4 years ago

@pckroon this breaks directly on TypeError: __new__() got multiple values for argument 'cls'

Its possible to change the parameter creation from

    params = symbols(names, cls=Parameter, seq=True, **kwargs)
    cls = kwargs.pop('cls', Parameter)
    params = symbols(names, cls=cls, seq=True, **kwargs)

To then get Dummy instances back, however they won't work because they don't have the required Parameter functionality (eg assign value/min/max)

Making Parameter a subclass of Dummy instead of Symbol does do the trick, seems to pass all tests also. Is there a reason to want Symbol behaviour over Dummy behaviour?

Jhsmit commented 4 years ago

More support for Dummies:

https://github.com/tBuLi/symfit/blob/ecc567a1e75092dc3c94cecb0b44c132dbab7f4e/symfit/core/models.py#L120-L123

tBuLi commented 4 years ago

Is there a reason to want Symbol behaviour over Dummy behaviour?

This is a very good question. I believe that if we coded everything correctly, there shouldn't be any difference since we never really do thing like Symbol('x') == Symbol('x') in symfits core. But changing that globally is a pretty big API change which could change some behavior. Although like you notice, all the test still work and the current Arguments can also be initiated without a name which is meant to achieve the same thing but does so in a worse way.

So thinking about it now, there is actually a lot to be said for this. My biggest concern is actually not the impact on symfit functions, but how these objects would interact with sympy itself.

Jhsmit commented 4 years ago

One way I can think of this going wrong is the user defining a parameter once with a certain name and then doing the same thing again later with the expectation that this gives the same sympy symbol. But I find it very unlikely that a user would expect this behavior or would do something like that.

tBuLi commented 4 years ago

I also think that is bad practice, and it is much easier when coding to reuse the same symbol then to type Symbol('x') again. But I think introducing a DummyParameter is a fine solution, and then we could also get rid of the Parameter() syntax and replace it by DummyParameter() where needed.