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

Added metaclass to create dummy Paramter/Variables #298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added metaclass to create dummy Paramter/Variables #298

wants to merge 1 commit into from

Conversation

Jhsmit
Copy link
Collaborator

@Jhsmit Jhsmit commented Feb 7, 2020

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?)

@tBuLi
Copy link
Owner

tBuLi commented Mar 19, 2020

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?

@Jhsmit
Copy link
Collaborator Author

Jhsmit commented Mar 19, 2020

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.

@tBuLi
Copy link
Owner

tBuLi commented Mar 19, 2020

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.

@Jhsmit
Copy link
Collaborator Author

Jhsmit commented Mar 6, 2022

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants