Opened 4 years ago

Closed 4 years ago

#23418 closed enhancement (fixed)

put number fields in Fields().Infinite()

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-8.2
Component: number fields Keywords:
Cc: nthiery, tscrim Merged in:
Authors: Frédéric Chapoton, Vincent Delecroix Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2008ba1 (Commits, GitHub, GitLab) Commit: 2008ba1318c182056afdf860f2064f66d603d738
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

and the same for QQ of course.

This fixes a bug with Hom where the meet computed is a join category, in which case X is not a subclass of category.parent_class. This causes problems with the number fields meeting with matrix spaces.

Follow up at #24432.

Change History (29)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/23418
  • Commit set to 2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df

New commits:

2f98551trac 23418 fixing some doctests in rings and category folders

comment:2 Changed 4 years ago by git

  • Commit changed from 2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df to a06ebbcba1804cbc7bbf0f4cb347b1a022c27335

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

a06ebbctrac 23418 correct more doctests

comment:3 Changed 4 years ago by chapoton

This triggers some problem here:

sage -t --long src/sage/structure/parent.pyx  # 6 doctests failed

comment:4 Changed 4 years ago by chapoton

  • Cc nthiery added

It seems that the morphisms from Sets take over the morphisms from Rings..

comment:5 Changed 4 years ago by tscrim

Failures are related to this:

sage: x = QQ['x'].0
sage: t = abs(ZZ.random_element(10^6))
sage: K = NumberField(x^2 + 2*3*7*11, "a"+str(t))
sage: a = K.gen()

sage: Hom(K, a.matrix().parent())
Set of Morphisms from Number Field in a791947 with defining polynomial x^2 + 462 to Full MatrixSpace of 2 by 2 dense matrices over Rational Field in Category of infinite rings
<class 'sage.categories.homset.Homset_with_category'>

compared to vanilla Sage:

sage: Hom(K, a.matrix().parent())
Set of Homomorphisms from Number Field in a395598 with defining polynomial x^2 + 462 to Full MatrixSpace of 2 by 2 dense matrices over Rational Field
sage: type(Hom(K, a.matrix().parent()))
<class 'sage.rings.homset.RingHomset_generic_with_category'>

This differences comes from the fact that the category becomes a join category (in the branch) because that is the meet category computed by Hom:

sage: K.category()._meet_(a.matrix().parent().category())
Category of infinite rings
sage: type(_)
<class 'sage.categories.category.JoinCategory_with_category'>

versus vanilla:

sage: K.category()._meet_(a.matrix().parent().category())
Category of rings
sage: type(_)
<class 'sage.categories.rings.Rings_with_category'>

Now, K is not a subclass of the meet category's parent_class, which causes the Cat.parent_class._Hom_ method to now raise an error:

sage: Cat = K.category()._meet_(a.matrix().parent().category())
sage: Cat.parent_class
<class 'sage.categories.category.JoinCategory.parent_class'>
sage: Cat.parent_class._Hom_(K, a.matrix().parent(), Cat)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-6-aa7f8d9cdf73> in <module>()
----> 1 Cat.parent_class._Hom_(K, a.matrix().parent(), Cat)
TypeError: unbound method _Hom_() must be called with JoinCategory.parent_class instance as first argument (got NumberField_quadratic_with_category instance instead)

versus vanilla

sage: Cat = K.category()._meet_(a.matrix().parent().category())
sage: Cat.parent_class
<class 'sage.categories.rings.Rings.parent_class'>
sage: Cat.parent_class._Hom_(K, a.matrix().parent(), Cat)
Set of Homomorphisms from Number Field in a395598 with defining polynomial x^2 + 462 to Full MatrixSpace of 2 by 2 dense matrices over Rational Field
Last edited 4 years ago by tscrim (previous) (diff)

comment:6 Changed 4 years ago by chapoton

  • Branch changed from u/chapoton/23418 to u/chapoton/23418_bare
  • Commit changed from a06ebbcba1804cbc7bbf0f4cb347b1a022c27335 to ba2ec5b2b3f1f5001541fcfeaea9597d4acc7866

New commits:

ba2ec5bMerge commit '2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df' in 8.1.b5

comment:7 Changed 4 years ago by git

  • Commit changed from ba2ec5b2b3f1f5001541fcfeaea9597d4acc7866 to 4e88b49d6818f5ef6abd61900a6e23024f576121

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

4e88b49fix

comment:8 Changed 4 years ago by git

  • Commit changed from 4e88b49d6818f5ef6abd61900a6e23024f576121 to 6d3e53209220b9d5132a2eaa583e14306c3d92fd

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

6d3e532trac 23418 fix

comment:9 Changed 4 years ago by vdelecroix

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Vincent Delecroix
  • Branch changed from u/chapoton/23418_bare to u/vdelecroix/23418
  • Commit changed from 6d3e53209220b9d5132a2eaa583e14306c3d92fd to d2bf048cc8da72b16e8c020d8c5fb6aa3b9ee923
  • Description modified (diff)
  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from new to needs_review

New commits:

d2bf04823418: number fields are infinite

comment:10 Changed 4 years ago by vdelecroix

the problem from 5 comment:5 is solved in d2bf048 via

  • src/sage/categories/homset.py

    a b def Hom(X, Y, category=None, check=True): 
    402402                #   will be called twice.
    403403                # - This is bound to fail if X is an extension type and
    404404                #   does not actually inherit from category.parent_class
    405                 H = category.parent_class._Hom_(X, Y, category = category)
     405                H = X.category().parent_class._Hom_(X, Y, category = category)
    406406            except (AttributeError, TypeError):
    407407                # By default, construct a plain homset.
    408408                H = Homset(X, Y, category = category, check=check)
Version 0, edited 4 years ago by vdelecroix (next)

comment:11 Changed 4 years ago by git

  • Commit changed from d2bf048cc8da72b16e8c020d8c5fb6aa3b9ee923 to a632bf6ffe46d7cdb412a4504200e047e5852322

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a632bf623418: number fields are infinite

comment:12 Changed 4 years ago by chapoton

  • Cc tscrim added

Looks good to me. Maybe Travis can confirm ?

comment:13 follow-up: Changed 4 years ago by tscrim

  • Status changed from needs_review to needs_work

No, I don't think this is correct because now it ignores the category input, and there might be a situation where this works with category being passed in (say, Modules() instead of Algebras()). So this change is a regression in my mind because the Homset classes do not have the inheritance of the categories. (In fact, in some ways, the error is because of this desired behavior.) Perhaps what we can do is do something special for join categories and testing its super_categories?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by vdelecroix

Replying to tscrim:

No, I don't think this is correct because now it ignores the category input

Strictly speaking it does not, since the category argument is passed in the call

H = X.category().parent_class._Hom_(X, Y, category = category)

and there might be a situation where this works with category being passed in (say, Modules() instead of Algebras()).

Do you have any example of this? I might understand the objection, but my "cheap fix" is indeed fixing problems and not creating new ones (as far as doctesting is complete).

So this change is a regression in my mind because the Homset classes do not have the inheritance of the categories. (In fact, in some ways, the error is because of this desired behavior.) Perhaps what we can do is do something special for join categories and testing its super_categories?

As you might have noticed, the call line 405 in categories/homset.py is already hackish. Parents implementing their own _Hom_ method should by themselves call a super (or equivalent). Before doing my "cheap fix", I thought that the good solution would be to remove this line 405 and fix the parents. Do you agree that this would be the solution?

If you agree with the above, there are two concrete ways of doing this:

  • let my cheap fix pass and remove line 405 afterwards
  • remove line 405 in a dependency ticket
Last edited 4 years ago by vdelecroix (previous) (diff)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by tscrim

Replying to vdelecroix:

Replying to tscrim:

No, I don't think this is correct because now it ignores the category input

Strictly speaking it does not, since the category argument is passed in the call

H = X.category().parent_class._Hom_(X, Y, category = category)

No, _Hom_ ignores the category input.

and there might be a situation where this works with category being passed in (say, Modules() instead of Algebras()).

Do you have any example of this? I might understand the objection, but my "cheap fix" is indeed fixing problems and not creating new ones (as far as doctesting is complete).

No, your hack is creating new problems. For example, suppose X is an algebra with:

  • basis {a, b},
  • multiplication a^2 = b^2 = ab = a.

If we want to construct a map f: X -> X given by a,b |-> b. This is not an algebra morphism, but instead only a module morphism. If we have Algebras._Hom_, which expects the category to be algebras (as it should), then this will break horribly and there is no way to use a Modules._Hom_ (assuming it is also implemented).

For an example where this is currently implemented, we can replace Algebras with HighestWeightCrystals and Modules with Crystals. I have also constructed similar morphisms as above for my research.

So this change is a regression in my mind because the Homset classes do not have the inheritance of the categories. (In fact, in some ways, the error is because of this desired behavior.) Perhaps what we can do is do something special for join categories and testing its super_categories?

As you might have noticed, the call line 405 in categories/homset.py is already hackish. Parents implementing their own _Hom_ method should by themselves call a super (or equivalent).

I am not sure this would be the same behavior. There are two different things going on:

  1. If a parent does not implement a _Hom_ method, then it should fall back to the specified category. Continuing my above example, this would only default to Algebras._Hom_, which would be wrong.
  2. The other behavior is if the homset returned by X._Hom_ does not support the more general category, say, because it takes advantage of the extra structure (say, is only uses the algebra generating set and not the [possibly infinite] basis). One should expect that X.category()._Hom_ would also take advantage of that extra structure as well and would similarly forbid more general categories.

Before doing my "cheap fix", I thought that the good solution would be to remove this line 405 and fix the parents. Do you agree that this would be the solution?

It doesn't make sense to me for the parent/category implementation of _Hom_ to do super calls when something fails. I think it would create a lot of redundant logic (the error handling). Also, everything in between X.category() and the specified category would likely (needlessly) fail, making these calls significantly slower when there is a large category difference. While this is a solution, I think it is a very bad solution.

I think special handling the join categories is going to be the proper fix as join categories are somewhat special in the category framework. While it may be just fixing the symptoms, it does not have the drawbacks that the other approaches have. I will push something today.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by vdelecroix

Replying to tscrim:

Replying to vdelecroix:

Replying to tscrim:

No, I don't think this is correct because now it ignores the category input

Strictly speaking it does not, since the category argument is passed in the call

H = X.category().parent_class._Hom_(X, Y, category = category)

No, _Hom_ ignores the category input.

and there might be a situation where this works with category being passed in (say, Modules() instead of Algebras()).

Do you have any example of this? I might understand the objection, but my "cheap fix" is indeed fixing problems and not creating new ones (as far as doctesting is complete).

No, your hack is creating new problems. [SNIP]

This would better be doctested! Could you come up with concrete examples to be added to the branch? Especially because we are going to play with the _Hom_ code.

So this change is a regression [...]

I will push something today.

Thanks! You might want to use another ticket or update the description of this one.

comment:17 in reply to: ↑ 16 Changed 4 years ago by tscrim

Replying to vdelecroix:

This would better be doctested! Could you come up with concrete examples to be added to the branch? Especially because we are going to play with the _Hom_ code.

I believe I have a concrete example and will add it.

So this change is a regression [...]

I will push something today.

Thanks! You might want to use another ticket or update the description of this one.

No problem. Thanks for your work on this.

comment:18 Changed 4 years ago by tscrim

  • Branch changed from u/vdelecroix/23418 to public/number_fields/in_infinite_fields-23418
  • Commit changed from a632bf6ffe46d7cdb412a4504200e047e5852322 to b7f3fa69fae7ca1119968e2805230a67d60c21d8
  • Description modified (diff)
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_work to needs_review

Actually, from looking at it and thinking about it more, the category of highest weight crystals is a full subcategory, so it does not quite work (in contrast to algebras and modules). So instead I just manufactured a minimal example that fails with your change.

Here is a branch that does special handling of JoinCategory. It passes all the tests that were previously failing.


New commits:

39b0560Merge branch 'u/vdelecroix/23418' of git://trac.sagemath.org/sage into public/number_fields/in_infinite_fields-23418
cdbc67dReverting change and adding test showing the behavior.
b7f3fa6Special handling of the join categories for Hom.

comment:19 Changed 4 years ago by vdelecroix

The following loop is randomly choosing the last working call.

for C in cats:
    try:
        H = C.parent_class._Hom_(X, Y, category=category)
    except (AttributeError, TypeError):
        pass

If you want to keep this hack, wouldn't it be better this way

for C in cats:
    try:
        H = C.parent_class._Hom_(X, Y, category=category)
    except (AttributeError, TypeError):
        pass
    else:
        break

comment:20 Changed 4 years ago by git

  • Commit changed from b7f3fa69fae7ca1119968e2805230a67d60c21d8 to bf01aa19df338890295dfe5ac99a1a300399f1cb

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

bf01aa1Break out once H is found.

comment:21 Changed 4 years ago by tscrim

Good point; fixed.

comment:22 Changed 4 years ago by tscrim

Green bot.

comment:23 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Sorry to have forgotten!

comment:24 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
git reset --hard Hsage -t --long --warn-long 69.0 src/sage/structure/parent.pyx
**********************************************************************
File "src/sage/structure/parent.pyx", line 1855, in sage.structure.parent.Parent.hom._generic_coerce_map
Failed example:
    QQ['x', 'y']._generic_coerce_map(QQ).category_for()
Expected:
    Category of unique factorization domains
Got:
    Category of infinite unique factorization domains
EAD~
**********************************************************************
1 item had failures:
   1 of   4 in sage.structure.parent.Parent.hom._generic_coerce_map
    [407 tests, 1 failure, 3.07 s]

comment:25 Changed 4 years ago by tscrim

I cannot reproduce this locally, so we might have to wait for the next beta. The doctest failure output should be the correct output. Yet, the test after it should probably also be Category of infinite euclidean domains, so something subtle might be going on too...

comment:26 Changed 4 years ago by git

  • Commit changed from bf01aa19df338890295dfe5ac99a1a300399f1cb to 2008ba1318c182056afdf860f2064f66d603d738

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

2714d24Merge branch 'public/number_fields/in_infinite_fields-23418' in 8.2.b3
2008ba1fix doctest

comment:27 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

I fixed the doctest

comment:28 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

So the failure was because of #24413, whereas the second test would need a similar treatment for power series rings. This is good enough for now.

comment:29 Changed 4 years ago by vbraun

  • Branch changed from public/number_fields/in_infinite_fields-23418 to 2008ba1318c182056afdf860f2064f66d603d738
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.