Opened 6 years ago
Closed 5 years ago
#23185 closed defect (fixed)
A polynomial ring embeds into its fraction field
Reported by:  Julian Rüth  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 6 years ago by
Description:  modified (diff) 

comment:2 Changed 6 years ago by
Description:  modified (diff) 

comment:3 Changed 6 years ago by
Dependencies:  → #23167 

comment:4 Changed 6 years ago by
comment:5 Changed 6 years ago by
Keywords:  sd86.5 added 

comment:6 Changed 6 years ago by
Description:  modified (diff) 

comment:7 Changed 6 years ago by
Description:  modified (diff) 

comment:8 Changed 5 years ago by
Branch:  → u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field 

comment:9 Changed 5 years ago by
Authors:  → Julian Rüth 

Commit:  → 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:  new → needs_review 

comment:12 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:13 Changed 5 years ago by
Dependencies:  #23167 → #23167, #23204 

comment:14 Changed 5 years ago by
Commit:  9f9a4994037de2265f08913afff5345be86556a1 → bacdac11f54c8afc23ff265c1fc98cba64a5f113 

Branch pushed to git repo; I updated commit sha1. New commits:
bacdac1  simplify equality checks

comment:15 Changed 5 years ago by
Commit:  bacdac11f54c8afc23ff265c1fc98cba64a5f113 → 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:  needs_work → needs_review 

comment:18 Changed 5 years ago by
Commit:  925e52abd082f87240348c0fafae892198364990 → 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:  1bf03cde52f076abdeda6d90d45d606e8768055f → 5074b2e2a628747bc436e47f5c0fbec5d83e9ef5 

Branch pushed to git repo; I updated commit sha1. New commits:
5074b2e  Fixed doctest

comment:20 Changed 5 years ago by
Status:  needs_review → needs_work 

Work issues:  → merge conflicts 
comment:21 Changed 5 years ago by
Commit:  5074b2e2a628747bc436e47f5c0fbec5d83e9ef5 → 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:  needs_work → needs_review 

Work issues:  merge conflicts 
comment:23 Changed 5 years ago by
Status:  needs_review → 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:  → failing doctests 

comment:25 Changed 5 years ago by
Commit:  d27ad10b0c8602e74b32fa047e25638614e6b191 → 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:  failing doctests → 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:  needs_work → needs_review 

Work issues:  check speed regression 
The tests pass for the files that failed before.
comment:29 Changed 5 years ago by
Commit:  faed28f4a7f263b8b84c78bbb96f28c8218ef583 → b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1 

comment:30 Changed 5 years ago by
Commit:  b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1 → 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:  → Marc Masdeu 

comment:33 Changed 5 years ago by
Status:  needs_review → needs_work 

Work issues:  → 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:  coverage in fraction_field.py → coverage in fraction_field.py, _richcmp_ is incorrect 

comment:36 Changed 5 years ago by
Work issues:  coverage in fraction_field.py, _richcmp_ is incorrect → coverage in fraction_field.py 

comment:37 Changed 5 years ago by
Commit:  79bac8fb2b798928270cd4d8d35bab8add32b124 → e3fb63156dd15edcf14bf670ba8ad5337cffffc7 

comment:38 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:  needs_work → needs_review 

Work issues:  coverage in fraction_field.py → waiting for the patchbot → positive review 
comment:40 Changed 5 years ago by
Work issues:  waiting for the patchbot → positive review 

comment:41 Changed 5 years ago by
Commit:  e3fb63156dd15edcf14bf670ba8ad5337cffffc7 → f3ecd0eeacb31055283b6edab35c8dcc178d96bd 

Branch pushed to git repo; I updated commit sha1. New commits:
f3ecd0e  added missing doctest

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:  needs_review → needs_work 

Work issues:  → 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:  #23167, #23204 → #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 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.
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:  #23167, #23204, #23529 → #23167, #23204 

comment:56 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 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 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:  f3ecd0eeacb31055283b6edab35c8dcc178d96bd → 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:  needs_work → needs_review 

Work issues:  handle <=,<,>=,>,… 
comment:61 Changed 5 years ago by
Branch:  u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field → u/roed/a_polynomial_ring_embeds_into_its_fraction_field 

comment:62 Changed 5 years ago by
Commit:  d5cd3c20eca8834d72e43dc25518ab74c2efe6dc → 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:  Marc Masdeu → Marc Masdeu, David Roe 

Status:  needs_review → positive_review 
comment:64 Changed 5 years ago by
Branch:  u/roed/a_polynomial_ring_embeds_into_its_fraction_field → fa2dcd05df27798908cb39eb734da8ff4a36c63e 

Resolution:  → fixed 
Status:  positive_review → closed 
Most likely, we need to patch
_coerce_map_from_
of function fields to return an injective homomorphism.