Opened 3 years ago

Closed 3 years ago

#26354 closed defect (fixed)

Pickling morphisms is broken

Reported by: caruso Owned by:
Priority: major Milestone: sage-8.4
Component: pickling Keywords:
Cc: Merged in:
Authors: David Roe, Xavier Caruso Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 1ea91f7 (Commits, GitHub, GitLab) Commit: 1ea91f7b6254f8791b4f0b488e32c002af7e99a0
Dependencies: Stopgaps:

Status badges

Description

Here is an exemple: 

sage: R.<x,y> = QQ[]
sage: theta = R.hom([y,x])
sage: hash(theta)
-1982612945128833994
sage: hash(loads(dumps(theta)))
Traceback (most recent call last):
...
ValueError: mutable sequences are unhashable

Change History (8)

comment:1 Changed 3 years ago by roed

  • Branch set to u/roed/pickling_morphisms_is_broken

comment:2 Changed 3 years ago by roed

  • Authors set to David Roe
  • Commit set to 94f6985f60c352044b4d99bf8fc15818c17c7281
  • Status changed from new to needs_review

New commits:

​94f6985Set immutable in unpickling ring morphisms

comment:3 Changed 3 years ago by tscrim

This is definitely a fix, but is it the correct fix? The PolynomialSequence does not seem to pickle immutability like I think it should: 

sage: theta.__reduce__()[1][3]['__im_gens']
[y, x]
sage: S = theta.__reduce__()[1][3]['__im_gens']
sage: type(S)
<class 'sage.rings.polynomial.multi_polynomial_sequence.PolynomialSequence_generic'>
sage: S.is_immutable()
True
sage: loads(dumps(S)).is_immutable()
False

Contrast: 

sage: S.__reduce__()
(<function PolynomialSequence at 0x7f62c7ef2c80>,
 (Multivariate Polynomial Ring in x, y over Rational Field, ((y, x),)))
sage: Sequence([1,2,3,4]).__reduce__()
(<function _reconstructor at 0x7f64198a5aa0>,
 (<class 'sage.structure.sequence.Sequence_generic'>,
  <type 'sage.structure.sage_object.SageObject'>,
  <sage.structure.sage_object.SageObject object at 0x7f62b02dc0b0>),
 {'_Sequence_generic__cr': False,
  '_Sequence_generic__cr_str': False,
  '_Sequence_generic__hash': None,
  '_Sequence_generic__universe': Integer Ring,
  '_is_immutable': False})

What are your thoughts about instead adjusting the pickling of PolynomialSequence?

comment:4 Changed 3 years ago by caruso

  • Branch changed from u/roed/pickling_morphisms_is_broken to u/caruso/pickling_morphisms_is_broken

comment:5 Changed 3 years ago by caruso

  • Commit changed from 94f6985f60c352044b4d99bf8fc15818c17c7281 to e2931b6b109cd7fb492945c93e90be7cd68a30bc

I agree with you, Travis.

I pushed a fix for the pickling of PolynomialSequence. I'm not sure I've done it correctly (in particular, this class redefines __getattr__, I don't know why); please review :-)

New commits:

​dee878eRevert "Set immutable in unpickling ring morphisms"
​e2931b6Fix pickling for PolynomialSequence_generic
Last edited 3 years ago by caruso (previous) (diff)

comment:6 Changed 3 years ago by git

  • Commit changed from e2931b6b109cd7fb492945c93e90be7cd68a30bc to 1ea91f7b6254f8791b4f0b488e32c002af7e99a0

Branch pushed to git repo; I updated commit sha1. New commits:

​1ea91f7Add a doctest

comment:7 Changed 3 years ago by tscrim

  • Authors changed from David Roe to David Roe, Xavier Caruso
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Yep, LGTM. Thanks.

comment:8 Changed 3 years ago by vbraun

  • Branch changed from u/caruso/pickling_morphisms_is_broken to 1ea91f7b6254f8791b4f0b488e32c002af7e99a0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.