Ticket #7251 (closed enhancement: fixed)

Opened 5 months ago

Last modified 5 months ago

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 Author(s): Nicolas M. Thiéry
Report Upstream: Reviewer(s): Mike Hansen
Merged in: sage-4.2.alpha1 Work issues:

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

trac_7251-integer_parent-nt.patch Download (3.1 KB) - added by nthiery 5 months ago.

Change History

  Changed 5 months ago by nthiery

  • owner changed from somebody to nthiery

follow-up: ↓ 3   Changed 5 months ago by robertwb

This will break the integer pool, as recycled integer might not have ZZ as their parent.

in reply to: ↑ 2   Changed 5 months ago by hivert

  • 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

follow-ups: ↓ 5 ↓ 7   Changed 5 months ago by 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

in reply to: ↑ 4   Changed 5 months ago by hivert

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

  Changed 5 months ago by mhansen

Haha -- I think it automagically copied / pasted for me.

in reply to: ↑ 4   Changed 5 months ago by nthiery

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)?

follow-up: ↓ 9   Changed 5 months ago by robertwb

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 5 months ago by nthiery

in reply to: ↑ 8   Changed 5 months ago by nthiery

  • 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.

  Changed 5 months ago by mhansen

  • status changed from needs_review to positive_review
  • reviewer set to Mike Hansen
  • merged set to sage-4.2.alpha1

Looks good to me and passes tests.

  Changed 5 months ago by mhansen

  • status changed from positive_review to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.