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

Allow declaration of Dummy arguments instead of just Symbols #290

Open
JohnGoertz opened this issue Jan 6, 2020 · 7 comments
Open

Allow declaration of Dummy arguments instead of just Symbols #290

JohnGoertz opened this issue Jan 6, 2020 · 7 comments

Comments

@JohnGoertz
Copy link

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
Copy link
Collaborator

pckroon commented Jan 6, 2020

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
Copy link
Collaborator

Jhsmit commented Feb 5, 2020

This was also discussed a while ago in #124

@Jhsmit
Copy link
Collaborator

Jhsmit commented Feb 6, 2020

@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
Copy link
Collaborator

Jhsmit commented Feb 6, 2020

More support for Dummies:

# TODO: this will break upon deprecating the auto-generation of
# names for Variables. At this time, a DummyVariable object
# should be introduced to fulfill the same role.
model = {Variable(): expr for expr in model}

@tBuLi
Copy link
Owner

tBuLi commented Mar 19, 2020

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
Copy link
Collaborator

Jhsmit commented Mar 19, 2020

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
Copy link
Owner

tBuLi commented Mar 20, 2020

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.

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

4 participants