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

Categories for padics, schemes, and some more rings #12877

Closed
nthiery opened this issue Apr 24, 2012 · 14 comments
Closed

Categories for padics, schemes, and some more rings #12877

nthiery opened this issue Apr 24, 2012 · 14 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Apr 24, 2012

This patch fixes the following classes to use categories:

  • padics
  • RealLazyField, ComplexLazyField
  • AlgebraicScheme's and subclasses

#11935 depends on this one

CC: @sagetrac-sage-combinat

Component: categories

Author: Nicolas M. Thiéry

Reviewer: Simon King

Merged: sage-5.1.beta1

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

@nthiery nthiery added this to the sage-5.1 milestone Apr 24, 2012
@nthiery nthiery self-assigned this Apr 24, 2012
@simon-king-jena
Copy link
Member

comment:2

Attachment: trac_12877-category-for_more_rings_and_schemes-nt.patch.gz

Hooray, finally the category of schemes is used in Sage! Question: Are there morphisms of schemes, yet? I am first reviewing #12875 and will then also look at the ticket here.

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2012

comment:3

Replying to @simon-king-jena:

Hooray, finally the category of schemes is used in Sage! Question: Are there morphisms of schemes, yet? I am first reviewing #12875 and will then also look at the ticket here.

Apparently yes :-)

sage: sage: k = GF(11)
sage: E = EllipticCurve(k,[1,1])
sage: Q = E(6,5)
sage: phi = E.isogeny(Q)
sage: phi.parent()
Set of Morphisms from Abelian group of points on Elliptic Curve defined by y^2 = x^3 + x + 1 over Finite Field of size 11 to Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 7*x + 8 over Finite Field of size 11 in Category of hom sets in Category of Schemes
sage: phi.parent().homset_category()
Category of hom sets in Category of Schemes

(which by the way should really be Category of Schemes; see #12880)

@simon-king-jena
Copy link
Member

comment:4

As you point out in a comment, due to the custom __getattr__ method, subclasses of LazyField can not be extension classes (because Parent.__getattr__ is only needed when the class of the parent can not be turned into a subclass of the category's parent_class).

I am sure that you did try something like getattr(super(self,LazyField),name). Did that not work? Why?

By consequence, you (have to) turn cdef class ComplexLazyField_class(LazyField) into class ComplexLazyField_class(LazyField). Did you investigate potential speed penalties?

In the initialisation of LocalGeneric, which inherits from CommutativeRing, you still call Parent.__init__ (with category support added), but not CommutativeRing.__init__, which would automatically grant category support. Why?

@simon-king-jena
Copy link
Member

comment:5

Replying to @simon-king-jena:

I am sure that you did try something like getattr(super(self,LazyField),name). Did that not work? Why?

I tested (it should be super(LazyField,self)), and indeed it does not work. But still, one could add the stuff from the custom Parent.__getattr__ into the custom LazyField.__getattr__:

sage: cython("""         
....: from sage.structure.parent cimport Parent
....: from sage.structure.parent import getattr_from_other_class
....: cdef class MyParent(Parent):
....:     def __getattr__(self, name):
....:         print "here is the custom getattr with", name
....:         return getattr_from_other_class(self, self.category().parent_class, name)
....: """)
sage: P = MyParent(category=CommutativeAdditiveSemigroups())
here is the custom getattr with _element_constructor_
here is the custom getattr with Element
here is the custom getattr with element_class
sage: P.addition_table
here is the custom getattr with addition_table
here is the custom getattr with __custom_name
here is the custom getattr with _repr_
<bound method CommutativeAdditiveSemigroups.parent_class.addition_table of <type '_mnt_local_king__sage_temp_mpc622_12258_tmp_4_spyx_0.MyParent'>>

There is also some more stuff in Parent.__getattr__, that was written for cached methods.

The question is whether the code duplication would be worth the effort.

@simon-king-jena
Copy link
Member

comment:6

But it is really strange:

sage: MyParent.__getattr__  
<method '__getattr__' of '_mnt_local_king__sage_temp_mpc622_12258_tmp_4_spyx_0.MyParent' objects>
sage: Parent.__getattr__  
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/mnt/local/king/SAGE/experiment/notebook/sage-5.1.notebook/devel/sage-main/<ipython console> in <module>()

AttributeError: type object 'sage.structure.parent.Parent' has no attribute '__getattr__'

So, why is Parent.__getattr__ seemingly unavailable, although it is frequently used?

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2012

comment:7

Thanks for reproducing here my unsuccessful attempts :-)

Now is it worth the trouble? It's about the parent, not the elements. In the similar situation for QQ we decided for QQ not to be an extension type. It would be surprising if there would be methods on RLF that would be more critical than on QQ, wouldn't it?

Cheers,
Nicolas

@simon-king-jena
Copy link
Member

comment:8

Replying to @nthiery:

Thanks for reproducing here my unsuccessful attempts :-)

You're welcome; but still I'd like to understand why one can see __getattr__ of a custom cdef class that is derived from Parent, but can not see __getattr__ of Parent.

@simon-king-jena
Copy link
Member

comment:9

There only remains one question: Why is Parent.__init__ and not CommutativeRing.__init__ called in LocalGeneric? Well, I'll test it, and will submit a reviewer patch if it works. With your patch, all doctests pass...

@simon-king-jena
Copy link
Member

Reviewer: Simon King

@simon-king-jena
Copy link
Member

comment:10

I see. Keywords such as element_constructor aren't accepted by CommutativeRing.__init__. So, no reviewer patch.

I accept your argument about RLF not being of extension type. Since the doctests pass and the patch looks fine, I give it a positive review.

However, I'd like to understand how Parent differs from other classes with a custom __getattr__.

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2012

comment:11

Replying to @simon-king-jena:

I accept your argument about RLF not being of extension type. Since the doctests pass and the patch looks fine, I give it a positive review.

Thanks for the quick review!

However, I'd like to understand how Parent differs from other classes with a custom __getattr__.

I guess it's a question of extension types versus non extension types.

@simon-king-jena
Copy link
Member

comment:12

Replying to @nthiery:

I guess it's a question of extension types versus non extension types.

No, it isn't.

sage: cython("""                  
....: cdef class MyParent(object):               
....:     def __getattr__(self, str name):
....:         raise AttributeError,name
....: """)
sage: MyParent.__getattr__
<method '__getattr__' of '_mnt_local_king__sage_temp_mpc622_20365_tmp_0_spyx_0.MyParent' objects>

At least that is what I find with sage-5.1.notebook. I do get an error in an old version of Sage that still uses the old Cython spkg.

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2012

comment:13

Replying to @simon-king-jena:

Replying to @nthiery:

I guess it's a question of extension types versus non extension types.

No, it isn't.

Ouch, that's weird ...

@jdemeyer
Copy link

Merged: sage-5.1.beta1

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

3 participants