Opened 5 years ago
Closed 4 years ago
#23418 closed enhancement (fixed)
put number fields in Fields().Infinite()
Reported by:  chapoton  Owned by:  

Priority:  minor  Milestone:  sage8.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: 
Description (last modified by )
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 5 years ago by
 Branch set to u/chapoton/23418
 Commit set to 2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df
comment:2 Changed 5 years ago by
 Commit changed from 2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df to a06ebbcba1804cbc7bbf0f4cb347b1a022c27335
Branch pushed to git repo; I updated commit sha1. New commits:
a06ebbc  trac 23418 correct more doctests

comment:3 Changed 5 years ago by
This triggers some problem here:
sage t long src/sage/structure/parent.pyx # 6 doctests failed
comment:4 Changed 5 years ago by
 Cc nthiery added
It seems that the morphisms from Sets take over the morphisms from Rings..
comment:5 Changed 5 years ago by
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) <ipythoninput6aa7f8d9cdf73> 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
comment:6 Changed 5 years ago by
 Branch changed from u/chapoton/23418 to u/chapoton/23418_bare
 Commit changed from a06ebbcba1804cbc7bbf0f4cb347b1a022c27335 to ba2ec5b2b3f1f5001541fcfeaea9597d4acc7866
New commits:
ba2ec5b  Merge commit '2f98551bf0aa8fc99f98cf2cdecf2fdaf584e5df' in 8.1.b5

comment:7 Changed 5 years ago by
 Commit changed from ba2ec5b2b3f1f5001541fcfeaea9597d4acc7866 to 4e88b49d6818f5ef6abd61900a6e23024f576121
Branch pushed to git repo; I updated commit sha1. New commits:
4e88b49  fix

comment:8 Changed 5 years ago by
 Commit changed from 4e88b49d6818f5ef6abd61900a6e23024f576121 to 6d3e53209220b9d5132a2eaa583e14306c3d92fd
Branch pushed to git repo; I updated commit sha1. New commits:
6d3e532  trac 23418 fix

comment:9 Changed 4 years ago by
 Branch changed from u/chapoton/23418_bare to u/vdelecroix/23418
 Commit changed from 6d3e53209220b9d5132a2eaa583e14306c3d92fd to d2bf048cc8da72b16e8c020d8c5fb6aa3b9ee923
 Description modified (diff)
 Milestone changed from sage8.1 to sage8.2
 Status changed from new to needs_review
New commits:
d2bf048  23418: number fields are infinite

comment:10 Changed 4 years ago by
the problem from comment:5 is solved in d2bf048
via

src/sage/categories/homset.py
a b def Hom(X, Y, category=None, check=True): 402 402 # will be called twice. 403 403 #  This is bound to fail if X is an extension type and 404 404 # 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) 406 406 except (AttributeError, TypeError): 407 407 # By default, construct a plain homset. 408 408 H = Homset(X, Y, category = category, check=check)
comment:11 Changed 4 years ago by
 Commit changed from d2bf048cc8da72b16e8c020d8c5fb6aa3b9ee923 to a632bf6ffe46d7cdb412a4504200e047e5852322
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a632bf6  23418: number fields are infinite

comment:13 followup: ↓ 14 Changed 4 years ago by
 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 ; followup: ↓ 15 Changed 4 years ago by
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 ofAlgebras()
).
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 way of doing this:
 let my cheap fix pass and remove line 405 afterwards
 remove line 405 in a dependency ticket
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 4 years ago by
Replying to vdelecroix:
Replying to tscrim:
No, I don't think this is correct because now it ignores the
category
inputStrictly speaking it does not, since the
category
argument is passed in the callH = 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 ofAlgebras()
).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:
 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 toAlgebras._Hom_
, which would be wrong.  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 thatX.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 ; followup: ↓ 17 Changed 4 years ago by
Replying to tscrim:
Replying to vdelecroix:
Replying to tscrim:
No, I don't think this is correct because now it ignores the
category
inputStrictly speaking it does not, since the
category
argument is passed in the callH = 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 ofAlgebras()
).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
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
 Branch changed from u/vdelecroix/23418 to public/number_fields/in_infinite_fields23418
 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:
39b0560  Merge branch 'u/vdelecroix/23418' of git://trac.sagemath.org/sage into public/number_fields/in_infinite_fields23418

cdbc67d  Reverting change and adding test showing the behavior.

b7f3fa6  Special handling of the join categories for Hom.

comment:19 Changed 4 years ago by
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
 Commit changed from b7f3fa69fae7ca1119968e2805230a67d60c21d8 to bf01aa19df338890295dfe5ac99a1a300399f1cb
Branch pushed to git repo; I updated commit sha1. New commits:
bf01aa1  Break out once H is found.

comment:21 Changed 4 years ago by
Good point; fixed.
comment:22 Changed 4 years ago by
Green bot.
comment:23 Changed 4 years ago by
 Status changed from needs_review to positive_review
Sorry to have forgotten!
comment:24 Changed 4 years ago by
 Status changed from positive_review to needs_work
git reset hard Hsage t long warnlong 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
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
 Commit changed from bf01aa19df338890295dfe5ac99a1a300399f1cb to 2008ba1318c182056afdf860f2064f66d603d738
comment:27 Changed 4 years ago by
 Status changed from needs_work to needs_review
I fixed the doctest
comment:28 Changed 4 years ago by
 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
 Branch changed from public/number_fields/in_infinite_fields23418 to 2008ba1318c182056afdf860f2064f66d603d738
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac 23418 fixing some doctests in rings and category folders