tBuLi / symfit

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

Make parameters fully pickleable #158

Closed pckroon closed 6 years ago

pckroon commented 6 years ago

It should be noted that the __getstate__ from the sympy parent classes is wrong, so I made a new one. Symbol.name survives pickling via the getnewargs method. We can't use that mechanism, since we need them to be passed to init.

tBuLi commented 6 years ago

I'm a bit surprised about the sympy getstate not being sufficient, because I think sympy objects are pickelable right? Do you have an explanation for this?

pckroon commented 6 years ago

If you have __slots__ you must also have a __gestate__ (and a __setstate__, bu there the Sympy's works: it just updates all the attributes after creation). Symbol.__getstate__ looks like this:

    def __getstate__(self):
        return {'_assumptions': self._assumptions}

Which of course is fine, if and only if you only have _assumptions. The rest of it's magice (name) has to be provided on object creation (not __init__, but __new__) which means this mechanism won't work. Instead, they're dealt with by __getnewargs__.

tBuLi commented 6 years ago

Thanks for the explanation.

And are you sure that the class attributes are basically broken by slots, and this global dict is the best workaround? If so, I will merge this.

tBuLi commented 6 years ago

whoops wrong button, premature close

pckroon commented 6 years ago

As it was it breaks on me saying that attribute _argument_index is read-only. This is I think the only workaround I saw. Maybe an alternative would be something like making a meta-class, or a singleton object that keeps track of this dict. Either way, overly complicated.

The only thing that's still troublesome is making more unnamed parameters after unpickling a bunch. But I'm not sure how big an issue that is.

tBuLi commented 6 years ago

That is indeed a bit troubling, but this is a deprecated feature anyway so I'm not sure how much time should be spend on it.

So I don't think pickling issues with automatically generated names are a big problem, since they are now against best practices anyway, so it's not unreasonable to demand people to be follow best practices when using multiprocessing for example. But would it be possible to modify __setstate__ to set argument_indices[cls] = max(argument_indices[cls], current_index)?

My biggest problem is that I want to avoid globals if in any way possible, so I would prefer any other solution over this but if there is no other way then we will do this since this feature is deprecated anyway.

pckroon commented 6 years ago

I'm not a fan of modifying setstate as you describe. It would require far too many braincycles to figure out how that namespace is going to work. I say we fix it when it breaks.

tBuLi commented 6 years ago

I agree with that.

But I just looked at the source of sympy.Dummy objects and they don't seem to have a problem with the class attribute, but it has to be a different one. See http://docs.sympy.org/1.0/_modules/sympy/core/symbol.html#Dummy.

Mimicking this is definitely the preferred solution.

pckroon commented 6 years ago

But that does mean your object will have a __dict__, negating some (all?) of the benefits involving __slots__

tBuLi commented 6 years ago

I finally had some time to check but Dummy's don't have __dict__'s, only __slots__. So I would prefer mimicking their solution to using a global variable.