Opened 5 years ago
Closed 5 years ago
#23185 closed defect (fixed)
A polynomial ring embeds into its fraction field
Reported by:  saraedum  Owned by:  

Priority:  minor  Milestone:  sage8.0 
Component:  commutative algebra  Keywords:  sd86.5, sd87 
Cc:  Merged in:  
Authors:  Julian Rüth  Reviewers:  Marc Masdeu, David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  fa2dcd0 (Commits, GitHub, GitLab)  Commit:  fa2dcd05df27798908cb39eb734da8ff4a36c63e 
Dependencies:  #23167, #23204  Stopgaps: 
Description (last modified by )
Currently, this fails:
sage: R.<x> = QQ[] sage: K.<x> = FunctionField(QQ) sage: R.is_subring(K) NotImplementedError sage: R.is_subring(R.fraction_field()) NotImplementedError
but it should return True
.
The latter could be implemented in the category of ring homomorphisms (to find out whether a map is a coercion, just ask for the coercion between those two rings.)
Change History (64)
comment:1 Changed 5 years ago by
 Description modified (diff)
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Dependencies set to #23167
comment:4 Changed 5 years ago by
comment:5 Changed 5 years ago by
 Keywords sd86.5 added
comment:6 Changed 5 years ago by
 Description modified (diff)
comment:7 Changed 5 years ago by
 Description modified (diff)
comment:8 Changed 5 years ago by
 Branch set to u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field
comment:9 Changed 5 years ago by
 Commit set to 9f9a4994037de2265f08913afff5345be86556a1
New commits:
9f9a499  A ring embeds into its fraction field

comment:10 Changed 5 years ago by
I expect quite a few doctest failures because error messages change.
comment:11 Changed 5 years ago by
 Status changed from new to needs_review
comment:12 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:13 Changed 5 years ago by
 Dependencies changed from #23167 to #23167, #23204
comment:14 Changed 5 years ago by
 Commit changed from 9f9a4994037de2265f08913afff5345be86556a1 to bacdac11f54c8afc23ff265c1fc98cba64a5f113
Branch pushed to git repo; I updated commit sha1. New commits:
bacdac1  simplify equality checks

comment:15 Changed 5 years ago by
 Commit changed from bacdac11f54c8afc23ff265c1fc98cba64a5f113 to 925e52abd082f87240348c0fafae892198364990
Branch pushed to git repo; I updated commit sha1. New commits:
925e52a  Merge remotetracking branch 'origin/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

comment:16 Changed 5 years ago by
 Keywords sd87 added
comment:17 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:18 Changed 5 years ago by
 Commit changed from 925e52abd082f87240348c0fafae892198364990 to 1bf03cde52f076abdeda6d90d45d606e8768055f
Branch pushed to git repo; I updated commit sha1. New commits:
2be1976  Merge remotetracking branch 'origin/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

1bf03cd  Merge branch 'u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field' of git://trac.sagemath.org/sage into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

comment:19 Changed 5 years ago by
 Commit changed from 1bf03cde52f076abdeda6d90d45d606e8768055f to 5074b2e2a628747bc436e47f5c0fbec5d83e9ef5
Branch pushed to git repo; I updated commit sha1. New commits:
5074b2e  Fixed doctest

comment:20 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to merge conflicts
comment:21 Changed 5 years ago by
 Commit changed from 5074b2e2a628747bc436e47f5c0fbec5d83e9ef5 to d27ad10b0c8602e74b32fa047e25638614e6b191
Branch pushed to git repo; I updated commit sha1. New commits:
d27ad10  Merge remotetracking branch 'trac/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

comment:22 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues merge conflicts deleted
comment:23 Changed 5 years ago by
 Status changed from needs_review to needs_work
This has a lot of doctests that fail:
sage t warnlong 100.3 src/sage/manifolds/differentiable/metric.py # 1 doctest failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/ell_rational_field.py # 5 doctests failed sage t warnlong 100.3 src/sage/symbolic/expression.pyx # 3 doctests failed sage t warnlong 100.3 src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py # 11 doctests failed sage t warnlong 100.3 src/sage/schemes/projective/projective_morphism.py # 38 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/sha_tate.py # 1 doctest failed sage t warnlong 100.3 src/sage/tests/gospersum.py # 12 doctests failed sage t warnlong 100.3 src/sage/modules/free_module_element.pyx # 16 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/padics.py # 2 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/ell_point.py # 5 doctests failed sage t warnlong 100.3 src/sage/modules/free_module.py # 8 doctests failed sage t warnlong 100.3 src/sage/schemes/curves/affine_curve.py # 16 doctests failed sage t warnlong 100.3 src/sage/combinat/perfect_matching.py # 2 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/constructor.py # 4 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/ell_egros.py # 3 doctests failed sage t warnlong 100.3 src/sage/schemes/projective/projective_point.py # 1 doctest failed sage t warnlong 100.3 src/sage/schemes/affine/affine_morphism.py # 29 doctests failed sage t warnlong 100.3 src/sage/rings/polynomial/polynomial_quotient_ring.py # 1 doctest failed sage t warnlong 100.3 src/sage/rings/morphism.pyx # 2 doctests failed sage t warnlong 100.3 src/sage/schemes/elliptic_curves/ell_tate_curve.py # 5 doctests failed sage t warnlong 100.3 src/sage/schemes/generic/morphism.py # 9 doctests failed sage t warnlong 100.3 src/sage/schemes/affine/affine_point.py # 6 doctests failed sage t warnlong 100.3 src/sage/rings/polynomial/skew_polynomial_element.pyx # 8 doctests failed sage t warnlong 100.3 src/sage/schemes/projective/endPN_minimal_model.py # 2 doctests failed sage t warnlong 100.3 src/sage/schemes/projective/projective_morphism_helper.pyx # 3 doctests failed
In particular, things that have nonunit denominators are failing to factor.
comment:24 Changed 5 years ago by
 Work issues set to failing doctests
comment:25 Changed 5 years ago by
 Commit changed from d27ad10b0c8602e74b32fa047e25638614e6b191 to faed28f4a7f263b8b84c78bbb96f28c8218ef583
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e891a88  Replace isinstance(RingHomomorphism)

0c3d772  fix doctests

b15febb  Fixed two doctests typos.

ea35cc4  Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion

961b615  Merge remotetracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion

d5affa7  The "morphism" might just be a "Map", namely a DefaultConvertMap

382e2d1  fix docbuild

05ee2ca  Merge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

1cf2b44  fix doctest output

faed28f  Accept keyword parameters

comment:26 Changed 5 years ago by
 Work issues changed from failing doctests to check speed regression
Last 10 new commits:
e891a88  Replace isinstance(RingHomomorphism)

0c3d772  fix doctests

b15febb  Fixed two doctests typos.

ea35cc4  Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion

961b615  Merge remotetracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion

d5affa7  The "morphism" might just be a "Map", namely a DefaultConvertMap

382e2d1  fix docbuild

05ee2ca  Merge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

1cf2b44  fix doctest output

faed28f  Accept keyword parameters

Last 10 new commits:
e891a88  Replace isinstance(RingHomomorphism)

0c3d772  fix doctests

b15febb  Fixed two doctests typos.

ea35cc4  Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion

961b615  Merge remotetracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion

d5affa7  The "morphism" might just be a "Map", namely a DefaultConvertMap

382e2d1  fix docbuild

05ee2ca  Merge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

1cf2b44  fix doctest output

faed28f  Accept keyword parameters

comment:27 Changed 5 years ago by
There is no speed regression:
sage: R.<x> = ZpCA(2)[] sage: K=R.fraction_field() sage: a = x/K(3) sage: %timeit R(a) 10000 loops, best of 3: 98.8 µs per loop # before 10000 loops, best of 3: 84.3 µs per loop # after sage: o = K.one() sage: %timeit R(o) 10000 loops, best of 3: 96.5 µs per loop # before 10000 loops, best of 3: 82.2 µs per loop # after
comment:28 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues check speed regression deleted
The tests pass for the files that failed before.
comment:29 Changed 5 years ago by
 Commit changed from faed28f4a7f263b8b84c78bbb96f28c8218ef583 to b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1
comment:30 Changed 5 years ago by
 Commit changed from b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1 to 79bac8fb2b798928270cd4d8d35bab8add32b124
comment:31 Changed 5 years ago by
The code looks good. Doctests in sage/rings pass. When the patchbot is happy I can give it a positive review.
comment:32 Changed 5 years ago by
 Reviewers set to Marc Masdeu
comment:33 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to coverage in fraction_field.py
comment:34 followup: ↓ 38 Changed 5 years ago by
Julian, quick (possibly stupid) question. In the class FractionFieldEmbeddingSection?, is _call_with_args doing anything that _call_ isn't? Currently it's the function lacking coverage in fraction_field.py, but maybe we don't need this function?
comment:35 Changed 5 years ago by
 Work issues changed from coverage in fraction_field.py to coverage in fraction_field.py, _richcmp_ is incorrect
comment:36 Changed 5 years ago by
 Work issues changed from coverage in fraction_field.py, _richcmp_ is incorrect to coverage in fraction_field.py
comment:37 Changed 5 years ago by
 Commit changed from 79bac8fb2b798928270cd4d8d35bab8add32b124 to e3fb63156dd15edcf14bf670ba8ad5337cffffc7
comment:38 in reply to: ↑ 34 Changed 5 years ago by
Replying to aly.deines:
Julian, quick (possibly stupid) question. In the class FractionFieldEmbeddingSection?, is _call_with_args doing anything that _call_ isn't? Currently it's the function lacking coverage in fraction_field.py, but maybe we don't need this function?
Some code in schemes does essentially:
sage: K = some_ring.fraction_field() sage: some_ring(K.some_element(), check=False)
I guess it was put there for performance reasons. The check
used to get passed on to the element_constructor
of R
. Now there is a proper map, so we need to consume the extra argument. This is done by implementing _call_with_args
. It does not do anything that _call_
does not but Parent
is calling _call_with_args
if there is any arguments. (I don't really know why Parent
can't just call _call_
with arguments.)
comment:39 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues changed from coverage in fraction_field.py to waiting for the patchbot → positive review
comment:40 Changed 5 years ago by
 Work issues waiting for the patchbot → positive review deleted
comment:41 Changed 5 years ago by
 Commit changed from e3fb63156dd15edcf14bf670ba8ad5337cffffc7 to f3ecd0eeacb31055283b6edab35c8dcc178d96bd
Branch pushed to git repo; I updated commit sha1. New commits:
f3ecd0e  added missing doctest

comment:42 Changed 5 years ago by
Looks good once tests pass.
comment:43 Changed 5 years ago by
One should avoid comparison of types. Using isinstance is the correct way of doing that.
comment:44 Changed 5 years ago by
chapoton: Do you refer to _richcmp_
? So, in general I agree that isinstance
is often better than equality of type. In this case, however, I don't see how someone would inherit from this class and mix this class and its subtype in the same homset – after all this is a coercion, so how should there be another coercion with the same domain and codomain around? At the same time, rewriting this to use isinstance
makes the code more complicated.
comment:45 Changed 5 years ago by
Well, I just wanted to say that comparison of types is not allowed in python3. I spend a lot of time trying to make progress towards python3, and seeing yet more work to be done makes me rather unhappy.
https://stackoverflow.com/questions/707674/howtocomparetypeofanobjectinpython
comment:46 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to handle <=,<,>=,>,…
Sure. I thought you were talking about ==
and !=
but of course we should do something about <=
and friends here.
comment:47 Changed 5 years ago by
 Dependencies changed from #23167, #23204 to #23167, #23204, #23529
comment:48 Changed 5 years ago by
There is no need for a new kind of richcmp variant. Just do not compare types, but use isinstance.
comment:49 Changed 5 years ago by
This is a very common task and it is apparently very easy to get wrong. So, I think there is a point in providing a helper that makes this easy to write (and mentioning it in Element._richcmp_
) I'd like to keep this one line of code without caring about what op
is exactly, so I'd like to have the new implementation be:
return richcmp_unordered((type(self),self.domain(),self.codomain()) == (type(other),other.domain(),other.codomain()), op)
Or do you have an idea for a similarly short implementation that gets all the cases right?
comment:50 followup: ↓ 51 Changed 5 years ago by
Actually
return richcmp_unordered(type(self) == type(other) and (self.domain(),self.codomain()) == (other.domain(),other.codomain()))
comment:51 in reply to: ↑ 50 Changed 5 years ago by
Replying to saraedum:
Actually
return richcmp_unordered(type(self) == type(other) and (self.domain(),self.codomain()) == (other.domain(),other.codomain()))
If you want to do just that (go through a boolean), then use rich_to_bool
In most of the cases, one really wants to compare (for inequalities) the rest, once the type is checked to be correct.
*EDIT* maybe rich_to_bool is not the correct solution here. It is rather used to take as input the result of an oldstyle cmp.
Why did you change the existing code in _richcmp_
? Did it give wrong results ?
comment:52 Changed 5 years ago by
Trying to find a solution that both of us would agree on, I would suggest :
0) Do no use your new proposal for a richmp_unordered here, but keep this ticket independent from that. You can then try elsewhere to convince that this is a good idea, without preventing the ticket here to be fixed.
1) use here the standard way to implement richcmp, looking like
if not isinstance(other, FractionFieldEmbeddingSection): return NotImplemented return richcmp((self.domain(), self.codomain()), (other.domain(), other.codomain()), op)
comment:53 Changed 5 years ago by
Ok. I think that I am fine with your proposal 1). The only thing I don't like about it is that if domain and codomain implement <=
for whatever reason, then self
will do as well. I am not sure if that's a good idea but I also don't mind.
comment:54 followups: ↓ 56 ↓ 57 Changed 5 years ago by
Shouldn't we raise a TypeError
instead of NotImplemented
? That seems to be what Python3 does:
>>> "" < 0 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: '<' not supported between instances of 'str' and 'int'
comment:55 Changed 5 years ago by
 Dependencies changed from #23167, #23204, #23529 to #23167, #23204
comment:56 in reply to: ↑ 54 Changed 5 years ago by
Replying to saraedum:
Shouldn't we raise a
TypeError
instead ofNotImplemented
? That seems to be what Python3 does:>>> "" < 0 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: '<' not supported between instances of 'str' and 'int'
No, I do not think so. It seems that NotImplemented? is what one should answer when not knowing how to compare. Python will know what to do with that, hopefuly.
comment:57 in reply to: ↑ 54 ; followup: ↓ 58 Changed 5 years ago by
Replying to saraedum:
Shouldn't we raise a
TypeError
instead ofNotImplemented
?
No and let me elaborate on that:
When doing A < B
, Python first calls A.__lt__(B)
(*). If that returns a value different from NotImplemented
, or if this raises an exception, this value or exception is the result of A < B
. On the other hand, if A.__lt__(B)
returns NotImplemented
, then Python calls B.__gt__(A)
(*). If that also returns NotImplemented
, then Python 3 raises a TypeError
. Python 2 falls back to __cmp__
instead.
What I just described is the Python behaviour. In this case, if the parents are not equal, the coercion model is involved too and that by itself does a fallback in case of NotImplemented
.
(*) NOTE: to be very precise, it actually does type(A).__lt__(A, B)
, but that is equivalent to A.__lt__(B)
, except that it does not look in A.__dict__
.
comment:58 in reply to: ↑ 57 Changed 5 years ago by
Thanks for explaining, I was not aware of that.
Replying to jdemeyer:
Replying to saraedum: When doing
A < B
, Python first callsA.__lt__(B)
(*). If that returns a value different fromNotImplemented
, or if this raises an exception, this value or exception is the result ofA < B
. On the other hand, ifA.__lt__(B)
returnsNotImplemented
, then Python callsB.__gt__(A)
(*). If that also returnsNotImplemented
, then Python 3 raises aTypeError
. Python 2 falls back to__cmp__
instead.
Does that not mean that we should favor
if type(self) != type(other): return NotImplemented …
over
if isinstance(other, MyType): return NotImplemented …
as the former means that a subclass can override _richcmp_
with a more specific check and it will always get called? Whereas in the latter it depends on the order of arguments what happens?
comment:59 Changed 5 years ago by
 Commit changed from f3ecd0eeacb31055283b6edab35c8dcc178d96bd to d5cd3c20eca8834d72e43dc25518ab74c2efe6dc
Branch pushed to git repo; I updated commit sha1. New commits:
d5cd3c2  Correctly raise NotImplemented in _richcmp_

comment:60 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues handle <=,<,>=,>,… deleted
comment:61 Changed 5 years ago by
 Branch changed from u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field to u/roed/a_polynomial_ring_embeds_into_its_fraction_field
comment:62 Changed 5 years ago by
 Commit changed from d5cd3c20eca8834d72e43dc25518ab74c2efe6dc to fa2dcd05df27798908cb39eb734da8ff4a36c63e
I've changed the hashes, since there could be multiple rings with the same fraction field. I ran tests, so if you're happy with these changes, feel free to give a positive review.
comment:63 Changed 5 years ago by
 Reviewers changed from Marc Masdeu to Marc Masdeu, David Roe
 Status changed from needs_review to positive_review
comment:64 Changed 5 years ago by
 Branch changed from u/roed/a_polynomial_ring_embeds_into_its_fraction_field to fa2dcd05df27798908cb39eb734da8ff4a36c63e
 Resolution set to fixed
 Status changed from positive_review to closed
Most likely, we need to patch
_coerce_map_from_
of function fields to return an injective homomorphism.