A polynomial ring embeds into its fraction field

[saraedum]

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.)

[saraedum]

Most likely, we need to patch _coerce_map_from_ of function fields to return an injective homomorphism.

[saraedum]

New commits:

​9f9a499

A ring embeds into its fraction field

[saraedum]

I expect quite a few doctest failures because error messages change.

[git]

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

​bacdac1

simplify equality checks

[git]

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

​925e52a

Merge remote-tracking branch 'origin/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

[git]

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

​2be1976	Merge remote-tracking 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

[git]

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

​5074b2e

Fixed doctest

[git]

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

​d27ad10

Merge remote-tracking branch 'trac/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field

[aly.deines]

This has a lot of doctests that fail:

sage -t --warn-long 100.3 src/sage/manifolds/differentiable/metric.py  # 1 doctest failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/ell_rational_field.py  # 5 doctests failed
sage -t --warn-long 100.3 src/sage/symbolic/expression.pyx  # 3 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py  # 11 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/projective/projective_morphism.py  # 38 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/sha_tate.py  # 1 doctest failed
sage -t --warn-long 100.3 src/sage/tests/gosper-sum.py  # 12 doctests failed
sage -t --warn-long 100.3 src/sage/modules/free_module_element.pyx  # 16 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/padics.py  # 2 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/ell_point.py  # 5 doctests failed
sage -t --warn-long 100.3 src/sage/modules/free_module.py  # 8 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/curves/affine_curve.py  # 16 doctests failed
sage -t --warn-long 100.3 src/sage/combinat/perfect_matching.py  # 2 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/constructor.py  # 4 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/ell_egros.py  # 3 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/projective/projective_point.py  # 1 doctest failed
sage -t --warn-long 100.3 src/sage/schemes/affine/affine_morphism.py  # 29 doctests failed
sage -t --warn-long 100.3 src/sage/rings/polynomial/polynomial_quotient_ring.py  # 1 doctest failed
sage -t --warn-long 100.3 src/sage/rings/morphism.pyx  # 2 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/elliptic_curves/ell_tate_curve.py  # 5 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/generic/morphism.py  # 9 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/affine/affine_point.py  # 6 doctests failed
sage -t --warn-long 100.3 src/sage/rings/polynomial/skew_polynomial_element.pyx  # 8 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/projective/endPN_minimal_model.py  # 2 doctests failed
sage -t --warn-long 100.3 src/sage/schemes/projective/projective_morphism_helper.pyx  # 3 doctests failed

In particular, things that have non-unit denominators are failing to factor.

[git]

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 remote-tracking 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

[saraedum]

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 remote-tracking 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 remote-tracking 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

[saraedum]

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

[saraedum]

The tests pass for the files that failed before.

[git]

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

​0ba6330	fix typo
​b2acd67	reverted to old exception messages in doctest

[git]

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

​04db251	fix whitespace
​79bac8f	fixed grammar

[mmasdeu]

The code looks good. Doctests in sage/rings pass. When the patchbot is happy I can give it a positive review.

[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?

[git]

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

​fb66f9f	Fix richcmp
​e3fb631	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

[saraedum]

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.)

[git]

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

​f3ecd0e

added missing doctest

[roed]

Looks good once tests pass.

[chapoton]

One should avoid comparison of types. Using isinstance is the correct way of doing that.

[saraedum]

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.

[chapoton]

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/how-to-compare-type-of-an-object-in-python

[saraedum]

Sure. I thought you were talking about == and != but of course we should do something about <= and friends here.

[chapoton]

There is no need for a new kind of richcmp variant. Just do not compare types, but use isinstance.

[saraedum]

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?

[saraedum]

Actually

return richcmp_unordered(type(self) == type(other) and (self.domain(),self.codomain()) == (other.domain(),other.codomain()))

[chapoton]

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.

[chapoton]

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)

[saraedum]

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.

[saraedum]

Shouldn't we raise a TypeError instead of NotImplemented? That seems to be what Python3 does:

>>> float <= int
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<=' not supported between instances of 'type' and 'type'

Then again this is for comparing types, not instances…

[chapoton]

Replying to saraedum:

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'

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.

[jdemeyer]

Replying to saraedum:

Shouldn't we raise a TypeError instead of NotImplemented?

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__.

[saraedum]

Thanks for explaining, I was not aware of that.

Replying to jdemeyer:

Replying to saraedum: 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.

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?

[git]

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

​d5cd3c2

Correctly raise NotImplemented in _richcmp_

[saraedum]

New commits:

​d5cd3c2

Correctly raise NotImplemented in _richcmp_

---

New commits:

​d5cd3c2

Correctly raise NotImplemented in _richcmp_

[roed]

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.