fraction field: fix conversion from symbolic ring

[dkrenn]

sage: z = SR.var('z')
sage: CyclotomicField(2)['z'].fraction_field()(2*(4*z + 5)/((z + 1)*(z - 1)^4))

raises an error but shouldn't. This issue is also mentioned on #24539.

AFAICS it worked in earlier SageMath versions, but probably due to #23664 (unchecked) it stopped working.

CC: @cheuberg @mezzarobba

Component: algebra

Author: Daniel Krenn

Branch/Commit: 1a3cc58

Reviewer: Marc Mezzarobba

Issue created by migration from https://trac.sagemath.org/ticket/26150

[dkrenn]

Branch: u/dkrenn/conv-fraction-field

[dkrenn]

Commit: f76254a

[dkrenn]

New commits:

f76254a

Trac #26150: fix conversion in fraction_field

[dkrenn]

Author: Daniel Krenn

[mezzarobba]

Reviewer: Marc Mezzarobba

[mezzarobba]

comment:4

Thanks for the fix! You can set the ticket to positive_review on my behalf if the patchbot is happy.

[dkrenn]

comment:5

Unfortunately, there is one doctest failing:

File "src/sage/rings/function_field/function_field.py", line 1602, in sage.rings.function_field.function_field.FunctionField_polymod.hom
Failed example:
    L.hom(r, base_morphism=phi)
Exception raised:
    Traceback (most recent call last):
    ...
    ValueError: invalid literal for int() with base 10: "Error in dput(length(sage1)) : object 'sage1' not foun"

I have no idea what is going on here.

[jdemeyer]

comment:6

Nitpick: :trac:`#26150` -> :trac:`26150`

[sagetrac-git]

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

b0920f3

fix trac ticket number reference in docstring

[sagetrac-git]

Changed commit from f76254a to b0920f3

[dkrenn]

comment:8

Replying to @jdemeyer:

Nitpick: :trac:`#26150` -> :trac:`26150`

Thanks. Changed.

[dkrenn]

comment:9

Replying to @dkrenn:

Unfortunately, there is one doctest failing:

File "src/sage/rings/function_field/function_field.py", line 1602, in sage.rings.function_field.function_field.FunctionField_polymod.hom
Failed example:
    L.hom(r, base_morphism=phi)
Exception raised:
    Traceback (most recent call last):
    ...
    ValueError: invalid literal for int() with base 10: "Error in dput(length(sage1)) : object 'sage1' not foun"

Follow up at #26155.

[sagetrac-git]

Changed commit from b0920f3 to f98b015

[sagetrac-git]

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

f98b015

Trac #26150: catch internal ValueError to get correct outer error response

[dkrenn]

comment:11

This is now an alternative fix for the issue. Needs review again and let's see what the patchbot says :)

[dkrenn]

comment:12

Again some failing doctests... :(

[sagetrac-git]

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

b31583a

Trac #26150: fix element_constructor of int(1)

[sagetrac-git]

Changed commit from f98b015 to b31583a

[sagetrac-git]

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

b3be7d9

Trac #26150: fix doctest (remove error message as something is now working)

[sagetrac-git]

Changed commit from b31583a to b3be7d9

[dkrenn]

comment:15

There is some underlying problem with a recursive call when creating an element out of int(1). This was somehow hidden before. I rewrote the element constructor a bit to adapt; also fixing an issue raised on #24539. The behavior is now more consistent with remaining SageMath.

So again, let us see what the patchbot says...

[sagetrac-git]

Changed commit from b3be7d9 to cdbed4f

[sagetrac-git]

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

cdbed4f

Merge tag '8.4.beta3' into t/26150/conv-fraction-field

[sagetrac-git]

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

2697d5c	Trac #26150: fix using correct one-element
45df8b5	Trac #26150: extend trying of decomposing the fraction

[sagetrac-git]

Changed commit from cdbed4f to 45df8b5

[dkrenn]

comment:18

Finally, I've written a fix that solves the issues and with which the patchbot is happy as well. To be more accurate, I've took the liberty to slightly rewrite the element constructor so that it covers a broader spectrum of inputs for the conversion. Now it should also be "correct" in the sense what it does at which point ;)

Please Review (again)

[dkrenn]

comment:19

@mezzarobba: You already have reviewed this ticket, but a small change was done after that. I would appreciate a review of the changes :)

[mezzarobba]

comment:20

Thanks for the notice.

I agree with most of the changes, but I'm not sure I like the unconditional conversions in resolve_fractions(). For which cases are they intended?

[sagetrac-git]

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

7625e86	Merge tag '8.7' into u/dkrenn/conv-fraction-field
1a3cc58	Trac #26150: add another doctest

[sagetrac-git]

Changed commit from 45df8b5 to 1a3cc58

[dkrenn]

comment:22

Replying to @mezzarobba:

Thanks for the notice.

I agree with most of the changes, but I'm not sure I like the unconditional conversions in resolve_fractions(). For which cases are they intended?

Added a doctest:

1a3cc58	Trac #26150: add another doctest

+            sage: T.<t> = ZZ[]
+            sage: S.<s> = ZZ[]
+            sage: S.fraction_field()(s/(s+1), (t-1)/(t+2))
+            (s^2 + 2*s)/(s^2 - 1)

Note that there is a conversion between S and T, so this conversion should be used also in the fraction field, at least in the case nothing else (proper coercion) works.

[mezzarobba]

comment:23

If you are really sure you want that, I won't oppose...

[vbraun]

Changed branch from u/dkrenn/conv-fraction-field to 1a3cc58