tBuLi / symfit

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

Pickle fails with sympy 1.9 #347

Closed kvr2007 closed 2 years ago

kvr2007 commented 2 years ago

First of all, many thanks for a great package! :)

I recently made a fresh installation of symfit in conda, and I cannot pickle an Argument object. This happens due to a failure of __getstate__ method:

>>> from symfit.core.argument import Argument
>>> a = Argument('test')
>>> a.__getstate__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "..../python3.9/site-packages/symfit/core/argument.py", line 69, in __getstate__
    state.update({slot: getattr(self, slot) for slot in self.__slots__
AttributeError: 'NoneType' object has no attribute 'update'

After some research I found that downgrading sympy from 1.9 to 1.5.1 fixes the problem:

>>> from symfit.core.argument import Argument
>>> a = Argument('test')
>>> a.__getstate__()
{'_assumptions': {'real': True, 'hermitian': True, 'commutative': True, 'infinite': False, 'extended_real': True, 'imaginary': False, 'complex': True, 'finite': True}, '_argument_index': 0}

Ultimately, this is related to the changes in sympy, which occurred between versions 1.5 and 1.9. In sympy 1.5 Symbol.__getstate__ is defined as follows:

return {'_assumptions': self._assumptions}

In sympy 1.9 this method returns None.

A simple fix would be to change line 66 in symfit/core/argument.py from state = super(Argument, self).__getstate__() to state = {'_assumptions': self._assumptions}, but I'm not sure if this is a correct way to approach this.

I am happy to provide more details!

Jhsmit commented 2 years ago

Awesome, that is the problem which has been bugging me for a while now. Thanks @kvr2007 for digging in and identifying the problem (and a solution)!

I'm not sure if the solution you propose is the correct one. Do we have tests for pickleability? Did you check if you can pickle and load the object correctly with those changes?

pckroon commented 2 years ago

I fixed this a while back in #342 I think, which is awaiting review.

On Thu, 6 Jan 2022, 12:55 Jochem Smit, @.***> wrote:

Awesome, that is the problem which has been bugging me for a while now. Thanks @kvr2007 https://github.com/kvr2007 for digging in and identifying the problem (and a solution)!

I'm not sure if the solution you propose is the correct one. Do we have tests for pickleability? Did you check if you can pickle and load the object correctly with those changes?

— Reply to this email directly, view it on GitHub https://github.com/tBuLi/symfit/issues/347#issuecomment-1006700686, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADC57OOBXK46NCTT3ZOQ5JLUUW3N3ANCNFSM5LMM3VJA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

Jhsmit commented 2 years ago

If I look at that code that seems like the proper fix I'll give you my +1 for that PR, perhaps our BDFL @tBuLi could have a look?

kvr2007 commented 2 years ago

Sorry, I only checked the open and closed issues, and overlooked that PR! If I add the __setstate__ method as well (using the code from PR #342 commit 8adcb2), then I can also unpickle with my solution. I also generated PKL files using both fixes in __getstate__ method, and the unpickled variables are identical (a == b returns True)