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

wrong category for parent of composition of number field endomorphisms #23647

Closed
fchapoton opened this issue Aug 18, 2017 · 27 comments
Closed

wrong category for parent of composition of number field endomorphisms #23647

fchapoton opened this issue Aug 18, 2017 · 27 comments

Comments

@fchapoton
Copy link
Contributor

namely

sage: K.<a, b> = NumberField([x^2 - 2, x^2 - 3])
sage: e, u, v, w = End(K)
sage: e.abs_hom().parent().category()
Category of homsets of number fields
sage: u.abs_hom().parent().category()
Category of homsets of number fields
sage: v.abs_hom().parent().category()
Category of homsets of number fields
sage: (v*v).abs_hom().parent().category()
Category of homsets of unital magmas and additive unital additive magmas

The last one should be the same as the other ones..

CC: @tscrim

Component: number fields

Author: Frédéric Chapoton

Branch/Commit: 768891b

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-8.1 milestone Aug 18, 2017
@fchapoton
Copy link
Contributor Author

comment:1

Travis, can you help, please ? This prevents from using richcmp for number fields morphisms.

@tscrim
Copy link
Collaborator

tscrim commented Aug 19, 2017

comment:2

So the problem comes down to this:

sage: v.parent() is End(K)
True
sage: v.parent() is Hom(K, K, category=NumberFields())
True
sage: (v*v).parent() == End(K)
True
sage: Hom(K, K, category=Rings()) is End(K)  # The issue
False
sage: Hom(K, K, category=Rings()) is (v*v).parent()
True

The source of which, when you go down _composition_, comes from this:

sage: v.category_for()  # Which calls
Category of rings
sage: v.parent().homset_category()
Category of rings

The latter should be NumberFields(), not Rings() because the subclass

sage.rings.number_field.morphism.NumberFieldHomset

does not pass NumberFields() as its category. However, I tried passing the category of the domain, but this causes failures because ComplexLazyField is not a number field and issues with CyclotomicField that I don't understand.

@fchapoton
Copy link
Contributor Author

comment:3

Do you have branch for what you tried ?

@tscrim
Copy link
Collaborator

tscrim commented Aug 30, 2017

comment:4

No, I don't as it didn't work. However, I basically just added this to number_field/morphism.py:

class NumberFieldHomset(RingHomset_generic):  # This was already there
    def __init__(self, R, S, category=None):
        if category is None:
            from sage.categories.number_fields import NumberFields
            category = NumberFields()
        RingHomset_generic(self, R, S, category)

and the other variant I tried was

    def __init__(self, R, S, category=None):
        if category is None:
            category = R.category()
        RingHomset_generic(self, R, S, category)

@fchapoton
Copy link
Contributor Author

comment:5

I have made another try, that causes interesting doctest failures:

Failed example:
    G = End(K); G
Expected:
    Automorphism group of Cyclotomic Field of order 12 and degree 4
Got:
    Set of Homomorphisms from Cyclotomic Field of order 12 and degree 4 to Cyclotomic Field of order 12 and degree 4

EDIT: ok, ok. This is indeed a group.

@fchapoton
Copy link
Contributor Author

comment:6

Here is my branch.


New commits:

ec77695tentative

@fchapoton
Copy link
Contributor Author

Commit: ec77695

@fchapoton
Copy link
Contributor Author

Branch: public/23647

@fchapoton
Copy link
Contributor Author

comment:7

in vanilla sage:

sage: End(QQ)
Set of Homomorphisms from Rational Field to Rational Field
sage: End(QuadraticField(7))
Automorphism group of Number Field in a with defining polynomial x^2 - 7
sage: End(CyclotomicField(7))
Automorphism group of Cyclotomic Field of order 7 and degree 6
sage: End(UniversalCyclotomicField())
Set of Homomorphisms from Universal Cyclotomic Field to Universal Cyclotomic Field
sage: End(GF(5))
Automorphism group of Finite Field of size 5

and

@tscrim
Copy link
Collaborator

tscrim commented Aug 30, 2017

comment:8

This does work, but it is a very big hack IMO. I think it might be worth trying to do a more comprehensive fix. At least as a first step, we can put QQ in NumberFields: #23761.

@tscrim
Copy link
Collaborator

tscrim commented Aug 30, 2017

comment:9

I believe UniversalCyclotomicField() is not a number field as I believe is an infinite-dimensional field extension of Q.

@fchapoton
Copy link
Contributor Author

comment:10

let us see what the patchbots say


New commits:

56c31aftrac 23647

@fchapoton
Copy link
Contributor Author

Changed branch from public/23647 to public/23647-v2

@fchapoton
Copy link
Contributor Author

Changed commit from ec77695 to 56c31af

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 17, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

49ddc5dtrac 23647 fix doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 17, 2017

Changed commit from 56c31af to 49ddc5d

@fchapoton
Copy link
Contributor Author

comment:12

This seems to work (somehow .__init__ was forgotten in the previous tentatives)

But

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

3676535trac 23647 better category handling in Number fields morphisms

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 49ddc5d to 3676535

@fchapoton
Copy link
Contributor Author

comment:14

Here is another tentative, let us see what patchbot says.


New commits:

3676535trac 23647 better category handling in Number fields morphisms

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 3676535 to 768891b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

768891btrac 23647 add doctest

@fchapoton
Copy link
Contributor Author

comment:17

Green patchbot. Travis, do you think that this is good enough ?

We could also try to find the best category given the target category..

@tscrim
Copy link
Collaborator

tscrim commented Sep 19, 2017

Author: Frédéric Chapoton

@tscrim
Copy link
Collaborator

tscrim commented Sep 19, 2017

comment:18

I think it improves things and is fairly close to a proper (IMO) fix of determining the category directly from the input. We can worry about doing more if this becomes an issue again somewhere else.

@tscrim
Copy link
Collaborator

tscrim commented Sep 19, 2017

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Sep 20, 2017

Changed branch from public/23647-v2 to 768891b

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