Skip to content

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Sep 23, 2025

fixes #28096.

The doctest failure https://github.com/sagemath/sage/actions/runs/17940271644/job/51014975901?pr=40874 is not related with this PR branch.

What the PR branch does is explained clearly in #28096 (comment)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@kwankyu kwankyu changed the title Cached methods with do_pickle=True ffor UniqueRepresentation Cached methods with do_pickle=True for UniqueRepresentation Sep 23, 2025
Copy link

github-actions bot commented Sep 23, 2025

Documentation preview for this PR (built with commit b0261ca; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu kwankyu marked this pull request as ready for review September 23, 2025 12:14
@kwankyu kwankyu marked this pull request as draft September 23, 2025 12:48
@kwankyu kwankyu marked this pull request as ready for review September 23, 2025 13:52
@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 23, 2025

About this comment: #28096 (comment) I think that there is no side effect except negligible increase of pickle size (due to added empty dict) for unique representation objects.

@vincentmacri
Copy link
Member

Thanks for getting this ready! This fixes a very subtle bug so I want to test locally (which will take a bit longer). I'll try to get to this in the next week or so.

I'm guessing from the genus function in your doctest that we might have encountered this bug in the same way (I found it while trying to cache the genus of a function field).

@vincentmacri
Copy link
Member

vincentmacri commented Sep 23, 2025

About this comment: #28096 (comment) I think that there is no side effect except negligible increase of pickle size (due to added empty dict) for unique representation objects.

I'm going to test this anyway, but I assume this doesn't change the ability to load in pickles from before this change?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 23, 2025

... I assume this doesn't change the ability to load in pickles from before this change?

I don't know. We need to test.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 23, 2025

... I'll try to get to this in the next week or so.

From the next week or so, I am out of town for a week or two. So take your time.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Sep 24, 2025

... I assume this doesn't change the ability to load in pickles from before this change?

I don't know. We need to test.

I tested. It is backward compatible.

On the develop branch:

sage: P.<a,b,c> = PolynomialRing(QQ,3, order='lex')
sage: I = sage.rings.ideal.Katsura(P,3)  
sage: I.groebner_basis()                     # @cached_method(do_pickle=True)
[a - 60*c^3 + 158/7*c^2 + 8/7*c - 1,
 b + 30*c^3 - 79/7*c^2 + 3/7*c,
 c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]
sage: I.save('test1')
sage: UCF = UniversalCyclotomicField()       # UniqueRepresentation
sage: UCF.zero()                             # @cached_method
0
sage: UCF.save('test2')                                                                                                                                  

Then on the PR branch:

sage: Ip = load('test1')
sage: Ip.groebner_basis.cache
{(('', None, None, False),
  ()): [a - 60*c^3 + 158/7*c^2 + 8/7*c - 1,
  b + 30*c^3 - 79/7*c^2 + 3/7*c,
  c^4 - 10/21*c^3 + 1/84*c^2 + 1/84*c]}
sage: P.<a,b,c> = PolynomialRing(QQ,3, order='lex')
sage: I = sage.rings.ideal.Katsura(P,3)
sage: I.groebner_basis.cache
{}
sage: Ip is I
False
sage: Ip == I
True
sage: UCFp  = load('test2')
sage: UCF = UniversalCyclotomicField()
sage: UCFp is UCF
True
sage: UCFp.zero.cache
sage: UCFp.zero()
0
sage: UCFp.zero.cache
0

By the way, I could not find a class with UniqueRepresentation and @cached_method(do_pickle=True). Of course, because it did not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cached methods with do_pickle=True fail for UniqueRepresentation objects
2 participants