Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for Integer(3, parent = MyParent) #7251

Closed
nthiery opened this issue Oct 19, 2009 · 11 comments
Closed

Allow for Integer(3, parent = MyParent) #7251

nthiery opened this issue Oct 19, 2009 · 11 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Oct 19, 2009

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.

CC: @sagetrac-sage-combinat @mwhansen @robertwb

Component: basic arithmetic

Keywords: Integer, IntegerWrapper

Author: Nicolas M. Thiéry

Reviewer: Mike Hansen

Merged: sage-4.2.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/7251

@nthiery nthiery added this to the sage-4.2 milestone Oct 19, 2009
@nthiery nthiery self-assigned this Oct 19, 2009
@robertwb
Copy link
Contributor

comment:2

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

@hivert
Copy link

hivert commented Oct 20, 2009

comment:3

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

@mwhansen
Copy link
Contributor

comment:4

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

@hivert
Copy link

hivert commented Oct 20, 2009

comment:5

Replying to @mwhansen:

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

@mwhansen
Copy link
Contributor

comment:6

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

@nthiery
Copy link
Contributor Author

nthiery commented Oct 20, 2009

comment:7

Replying to @mwhansen:

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

@robertwb
Copy link
Contributor

comment:8

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.

@nthiery
Copy link
Contributor Author

nthiery commented Oct 20, 2009

comment:9

Attachment: trac_7251-integer_parent-nt.patch.gz

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.

@mwhansen
Copy link
Contributor

comment:10

Looks good to me and passes tests.

@mwhansen
Copy link
Contributor

Reviewer: Mike Hansen

@mwhansen
Copy link
Contributor

Merged: sage-4.2.alpha1

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

No branches or pull requests

4 participants