symfit icon indicating copy to clipboard operation
symfit copied to clipboard

Allow declaration of Dummy arguments instead of just Symbols

Open JohnGoertz opened this issue 4 years ago • 7 comments

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.

JohnGoertz avatar Jan 06 '20 15:01 JohnGoertz

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?

pckroon avatar Jan 06 '20 16:01 pckroon

This was also discussed a while ago in #124

Jhsmit avatar Feb 05 '20 11:02 Jhsmit

@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 avatar Feb 06 '20 14:02 Jhsmit

More support for Dummies:

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

Jhsmit avatar Feb 06 '20 16:02 Jhsmit

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.

tBuLi avatar Mar 19 '20 14:03 tBuLi

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.

Jhsmit avatar Mar 19 '20 15:03 Jhsmit

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.

tBuLi avatar Mar 20 '20 10:03 tBuLi