Opened 7 years ago
Closed 7 years ago
#7251 closed enhancement (fixed)
Allow for Integer(3, parent = MyParent)
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-4.2 |
Component: | basic arithmetic | Keywords: | Integer, IntegerWrapper |
Cc: | sage-combinat, mhansen, robertwb | Merged in: | sage-4.2.alpha1 |
Authors: | Nicolas M. Thiéry | Reviewers: | Mike Hansen |
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The attached patch allows for the creation of integers whose parents are not IntegerRing?():
sage: n = Integer(3, parent = Primes()) sage: n 3 sage: n.parent() Set of all prime numbers: 2, 3, 5, 7, ...
That's used in a couple places in the category code #5891, when illustrating how to create new parents like the set of prime integers. So this is quite urgent.
Any better implementation welcome! I am fine also with having this work only for IntegerWrapper?.
Attachments (1)
Change History (12)
comment:1 Changed 7 years ago by
- Owner changed from somebody to nthiery
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
- Cc changed from sage-combinat,mhansen to sage-combinat, mhansen
Replying to robertwb:
This will break the integer pool, as recycled integer might not have ZZ as their parent.
Can you elaborate a little bit more. Is this a serious problem ? We definitely want to create parents whose element will be some kind of integers. Does sage definitely prevents to inherits from integer and change the parent ? Do we have to wrap the integer as an *attribute* ?
Cheers,
Florent
comment:4 follow-ups: ↓ 5 ↓ 7 Changed 7 years ago by
I may be mistaken, but I don't think the IntegerWrapper? objects use the integer pool. Robert? If that's the case, then that would be a solution.he most innovative decor on the island. Located in the heart of Phuket Town. An absoute 'must' if you're in town.
mid-range reservations recommended parking nearby smoking dj music credit cards accepted
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to mhansen:
I may be mistaken, but I don't think the IntegerWrapper? objects use the integer pool. Robert? If that's the case, then that would be a solution.he most innovative decor on the island. Located in the heart of Phuket Town. An absoute 'must' if you're in town.
mid-range reservations recommended parking nearby smoking dj music credit cards accepted
Is trac adding some kind of subliminal add ? :-)
Florent
comment:6 Changed 7 years ago by
Haha -- I think it automagically copied / pasted for me.
comment:7 in reply to: ↑ 4 Changed 7 years ago by
Replying to mhansen:
I may be mistaken, but I don't think the IntegerWrapper? objects use the integer pool. Robert?
I double checked our use cases, and altogether we indeed only need the feature
sage: IntegerWrapper(3, parent = Primes())
with the rest as in the description.
However, I have a problem, since if I move the init to IntegerWrapper?, then cinit of Integer still gets called, and complains about the parent argument. Robert, Mike: would you mind investigating what's the right way for implementing this (I am not so familiar with cython initialization)?
comment:8 follow-up: ↓ 9 Changed 7 years ago by
Yes, you should just use the wrapper (at least for now).
I should explain this a bit more--typically there shouldn't be any issues doing this with Cython classes. With Integers what we do is hijack the allocation/deallocation function slots with our own custom functions that stick already allocated Integers (with initialized parent and mpz_t fields) into a pool on "deallocation" and then pull them out whenever a new one is needed. Because Integers are so common, this is actually a significant savings, but does cause issues with subclassing from Python.
IntegerWrapper? is OK because it statically sets its alloc/dealloc methods to the *original* Integer alloc/dealloc methods (before we manually swap them out for ours).
It's all a bit messy, and I've been wanting to redo it for a while (extending it to a cleaner and more generic framework that can be used elsewhere) but I have much higher priority stuff (like a thesis) to be doing.
Changed 7 years ago by
comment:9 in reply to: ↑ 8 Changed 7 years ago by
- Cc robertwb added
- Status changed from new to needs_review
Replying to robertwb:
I should explain this a bit more--typically there shouldn't be any issues doing this with Cython classes. With Integers what we do is hijack the allocation/deallocation function slots with our own custom functions that stick already allocated Integers (with initialized parent and mpz_t fields) into a pool on "deallocation" and then pull them out whenever a new one is needed. Because Integers are so common, this is actually a significant savings, but does cause issues with subclassing from Python.
IntegerWrapper? is OK because it statically sets its alloc/dealloc methods to the *original* Integer alloc/dealloc methods (before we manually swap them out for ours).
Thanks for the explanations! I have added them to the documentation of IntegerWrapper?.
The latest patch also just modifies IntegerWrapper?, without touching Integer.
comment:10 Changed 7 years ago by
- Merged in set to sage-4.2.alpha1
- Reviewers set to Mike Hansen
- Status changed from needs_review to positive_review
Looks good to me and passes tests.
comment:11 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
This will break the integer pool, as recycled integer might not have ZZ as their parent.