#23185 closed defect (fixed)

A polynomial ring embeds into its fraction field

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-8.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) Commit: fa2dcd05df27798908cb39eb734da8ff4a36c63e
Dependencies: #23167, #23204 Stopgaps:

Description (last modified by 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.)

Change History (64)

comment:1 Changed 23 months ago by saraedum

  • Description modified (diff)

comment:2 Changed 23 months ago by saraedum

  • Description modified (diff)

comment:3 Changed 23 months ago by saraedum

  • Dependencies set to #23167

comment:4 Changed 23 months ago by saraedum

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

comment:5 Changed 23 months ago by saraedum

  • Keywords sd86.5 added

comment:6 Changed 23 months ago by saraedum

  • Description modified (diff)

comment:7 Changed 23 months ago by saraedum

  • Description modified (diff)

comment:8 Changed 22 months ago by saraedum

  • Branch set to u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field

comment:9 Changed 22 months ago by saraedum

  • Authors set to Julian Rüth
  • Commit set to 9f9a4994037de2265f08913afff5345be86556a1

New commits:

9f9a499A ring embeds into its fraction field

comment:10 Changed 22 months ago by saraedum

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

comment:11 Changed 22 months ago by saraedum

  • Status changed from new to needs_review

comment:12 Changed 22 months ago by saraedum

  • Status changed from needs_review to needs_work

comment:13 Changed 22 months ago by saraedum

  • Dependencies changed from #23167 to #23167, #23204

comment:14 Changed 22 months ago by git

  • Commit changed from 9f9a4994037de2265f08913afff5345be86556a1 to bacdac11f54c8afc23ff265c1fc98cba64a5f113

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

bacdac1simplify equality checks

comment:15 Changed 22 months ago by git

  • Commit changed from bacdac11f54c8afc23ff265c1fc98cba64a5f113 to 925e52abd082f87240348c0fafae892198364990

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

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

comment:16 Changed 21 months ago by roed

  • Keywords sd87 added

comment:17 Changed 21 months ago by saraedum

  • Status changed from needs_work to needs_review

comment:18 Changed 21 months ago by git

  • Commit changed from 925e52abd082f87240348c0fafae892198364990 to 1bf03cde52f076abdeda6d90d45d606e8768055f

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

2be1976Merge remote-tracking branch 'origin/develop' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field
1bf03cdMerge 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 21 months ago by git

  • Commit changed from 1bf03cde52f076abdeda6d90d45d606e8768055f to 5074b2e2a628747bc436e47f5c0fbec5d83e9ef5

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

5074b2eFixed doctest

comment:20 Changed 21 months ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflicts

comment:21 Changed 21 months ago by git

  • Commit changed from 5074b2e2a628747bc436e47f5c0fbec5d83e9ef5 to d27ad10b0c8602e74b32fa047e25638614e6b191

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

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

comment:22 Changed 21 months ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues merge conflicts deleted

comment:23 Changed 21 months ago by aly.deines

  • Status changed from needs_review to needs_work

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.

comment:24 Changed 21 months ago by saraedum

  • Work issues set to failing doctests

comment:25 Changed 21 months ago by git

  • Commit changed from d27ad10b0c8602e74b32fa047e25638614e6b191 to faed28f4a7f263b8b84c78bbb96f28c8218ef583

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e891a88Replace isinstance(RingHomomorphism)
0c3d772fix doctests
b15febbFixed two doctests typos.
ea35cc4Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
961b615Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion
d5affa7The "morphism" might just be a "Map", namely a DefaultConvertMap
382e2d1fix docbuild
05ee2caMerge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field
1cf2b44fix doctest output
faed28fAccept keyword parameters

comment:26 Changed 21 months ago by saraedum

  • Work issues changed from failing doctests to check speed regression

Last 10 new commits:

e891a88Replace isinstance(RingHomomorphism)
0c3d772fix doctests
b15febbFixed two doctests typos.
ea35cc4Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
961b615Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion
d5affa7The "morphism" might just be a "Map", namely a DefaultConvertMap
382e2d1fix docbuild
05ee2caMerge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field
1cf2b44fix doctest output
faed28fAccept keyword parameters

Last 10 new commits:

e891a88Replace isinstance(RingHomomorphism)
0c3d772fix doctests
b15febbFixed two doctests typos.
ea35cc4Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
961b615Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion
d5affa7The "morphism" might just be a "Map", namely a DefaultConvertMap
382e2d1fix docbuild
05ee2caMerge branch 't/23204/remove_ringhomomorphism_coercion' into t/23185/a_polynomial_ring_embeds_into_its_fraction_field
1cf2b44fix doctest output
faed28fAccept keyword parameters

comment:27 Changed 21 months ago by 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

comment:28 Changed 21 months ago by saraedum

  • 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 21 months ago by git

  • Commit changed from faed28f4a7f263b8b84c78bbb96f28c8218ef583 to b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1

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

0ba6330fix typo
b2acd67reverted to old exception messages in doctest

comment:30 Changed 21 months ago by git

  • Commit changed from b2acd67fd30e83d9efe75e01bb30c8cc0fea41e1 to 79bac8fb2b798928270cd4d8d35bab8add32b124

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

04db251fix whitespace
79bac8ffixed grammar

comment:31 Changed 21 months ago by mmasdeu

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

comment:32 Changed 21 months ago by mmasdeu

  • Reviewers set to Marc Masdeu

comment:33 Changed 21 months ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to coverage in fraction_field.py

comment:34 follow-up: Changed 21 months ago by 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?

comment:35 Changed 21 months ago by saraedum

  • Work issues changed from coverage in fraction_field.py to coverage in fraction_field.py, _richcmp_ is incorrect

comment:36 Changed 21 months ago by saraedum

  • Work issues changed from coverage in fraction_field.py, _richcmp_ is incorrect to coverage in fraction_field.py

comment:37 Changed 21 months ago by git

  • Commit changed from 79bac8fb2b798928270cd4d8d35bab8add32b124 to e3fb63156dd15edcf14bf670ba8ad5337cffffc7

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

fb66f9fFix richcmp
e3fb631Merge 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:38 in reply to: ↑ 34 Changed 21 months ago by 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.)

comment:39 Changed 21 months ago by saraedum

  • 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 21 months ago by saraedum

  • Work issues waiting for the patchbot → positive review deleted

comment:41 Changed 21 months ago by git

  • Commit changed from e3fb63156dd15edcf14bf670ba8ad5337cffffc7 to f3ecd0eeacb31055283b6edab35c8dcc178d96bd

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

f3ecd0eadded missing doctest

comment:42 Changed 21 months ago by roed

Looks good once tests pass.

comment:43 Changed 21 months ago by chapoton

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

comment:44 Changed 21 months ago by 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.

comment:45 Changed 21 months ago by 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

Last edited 21 months ago by chapoton (previous) (diff)

comment:46 Changed 21 months ago by saraedum

  • 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 21 months ago by saraedum

  • Dependencies changed from #23167, #23204 to #23167, #23204, #23529

comment:48 Changed 21 months ago by chapoton

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

comment:49 Changed 21 months ago by 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?

comment:50 follow-up: Changed 21 months ago by saraedum

Actually

return richcmp_unordered(type(self) == type(other) and (self.domain(),self.codomain()) == (other.domain(),other.codomain()))
Last edited 21 months ago by saraedum (previous) (diff)

comment:51 in reply to: ↑ 50 Changed 21 months ago by 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.

Last edited 21 months ago by chapoton (previous) (diff)

comment:52 Changed 21 months ago by 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)

comment:53 Changed 21 months ago by 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.

comment:54 follow-ups: Changed 21 months ago by 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'
Last edited 21 months ago by saraedum (previous) (diff)

comment:55 Changed 21 months ago by saraedum

  • Dependencies changed from #23167, #23204, #23529 to #23167, #23204

comment:56 in reply to: ↑ 54 Changed 21 months ago by 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.

comment:57 in reply to: ↑ 54 ; follow-up: Changed 21 months ago by 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__.

comment:58 in reply to: ↑ 57 Changed 21 months ago by 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?

comment:59 Changed 21 months ago by git

  • Commit changed from f3ecd0eeacb31055283b6edab35c8dcc178d96bd to d5cd3c20eca8834d72e43dc25518ab74c2efe6dc

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

d5cd3c2Correctly raise NotImplemented in _richcmp_

comment:60 Changed 21 months ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues handle <=,<,>=,>,… deleted

New commits:

d5cd3c2Correctly raise NotImplemented in _richcmp_

New commits:

d5cd3c2Correctly raise NotImplemented in _richcmp_

comment:61 Changed 21 months ago by roed

  • 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 21 months ago by roed

  • 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 21 months ago by saraedum

  • Reviewers changed from Marc Masdeu to Marc Masdeu, David Roe
  • Status changed from needs_review to positive_review

comment:64 Changed 21 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.