Opened 3 years ago

Closed 2 years ago

#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 3 years ago by saraedum

  • Description modified (diff)

comment:2 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:3 Changed 3 years ago by saraedum

  • Dependencies set to #23167

comment:4 Changed 3 years ago by saraedum

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

comment:5 Changed 3 years ago by saraedum

  • Keywords sd86.5 added

comment:6 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:7 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:8 Changed 2 years ago by saraedum

  • Branch set to u/saraedum/a_polynomial_ring_embeds_into_its_fraction_field

comment:9 Changed 2 years 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 2 years ago by saraedum

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

comment:11 Changed 2 years ago by saraedum

  • Status changed from new to needs_review

comment:12 Changed 2 years ago by saraedum

  • Status changed from needs_review to needs_work

comment:13 Changed 2 years ago by saraedum

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

comment:14 Changed 2 years 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 2 years 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 2 years ago by roed

  • Keywords sd87 added

comment:17 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:18 Changed 2 years 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 2 years 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 2 years ago by saraedum

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

comment:21 Changed 2 years 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 2 years ago by saraedum

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

comment:23 Changed 2 years 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 2 years ago by saraedum

  • Work issues set to failing doctests

comment:25 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by mmasdeu

  • Reviewers set to Marc Masdeu

comment:33 Changed 2 years 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 2 years 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 2 years ago by saraedum

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

comment:36 Changed 2 years ago by saraedum

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

comment:37 Changed 2 years 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 2 years 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 2 years 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 2 years ago by saraedum

  • Work issues waiting for the patchbot → positive review deleted

comment:41 Changed 2 years 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 2 years ago by roed

Looks good once tests pass.

comment:43 Changed 2 years ago by chapoton

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

comment:44 Changed 2 years 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 2 years 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 2 years ago by chapoton (previous) (diff)

comment:46 Changed 2 years 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 2 years ago by saraedum

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

comment:48 Changed 2 years 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 2 years 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 2 years ago by saraedum

Actually

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

comment:51 in reply to: ↑ 50 Changed 2 years 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 2 years ago by chapoton (previous) (diff)

comment:52 Changed 2 years 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 2 years 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 2 years 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 2 years ago by saraedum (previous) (diff)

comment:55 Changed 2 years ago by saraedum

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

comment:56 in reply to: ↑ 54 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by saraedum

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

comment:64 Changed 2 years 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.