#29713 closed defect (fixed)

Broken conversion from FractionField over PolynomialRing over Field back to Field

Reported by: gh-petRUShka Owned by:
Priority: major Milestone: sage-9.2
Component: algebra Keywords: fraction field
Cc: mmezzarobba, caruso, slelievre, saraedum, vdelecroix Merged in:
Authors: Travis Scrimshaw Reviewers: Xavier Caruso
Report Upstream: N/A Work issues:
Branch: 7de0300 (Commits, GitHub, GitLab) Commit: 7de03008dc6315bf93919e784697f112a493cd76
Dependencies: Stopgaps:

Status badges

Description

Let consider following code:

F = FunctionField(QQ, 'a')
a = F.gen()

polynomial_ring = PolynomialRing(F, 'x')
FF = FractionField(polynomial_ring)

element = F(-1/2/(a^2 + a))

assert F(FF(element))

The last line yields following error:

>           raise TypeError("fraction must have unit denominator")
E           TypeError: fraction must have unit denominator

/usr/lib/python3.8/site-packages/sage/rings/fraction_field.py:1190: TypeError

But actually it should be just conversion from FractionField? element back to Field.

P.S. There is also one more thing:

assert FF(element) in F

yields following:

E       assert -1/2/(a^2 + a) in Rational function field in a over Rational Field
E        +  where -1/2/(a^2 + a) = Fraction Field of Univariate Polynomial Ring in x over Rational function field in a over Rational Field(-1/2/(a^2 + a))

P.P.S. SageMath version 9.0, Release Date: 2020-01-01

Change History (13)

comment:1 Changed 17 months ago by heluani

  • Branch set to u/heluani/29713
  • Commit set to 831cfa6b6273f1abf44ba13759b5edd51745cd3c
  • Status changed from new to needs_review

I feel like the right solution is to have FractionFieldEmbeddingSection?._call_ raise a ValueError instead of a TypeError. From the code in fraction_field.py:

     # This should probably be a ValueError.
     # However, too much existing code is expecting this to throw a
     # TypeError, so we decided to keep it for the time being.
     raise TypeError("fraction must have unit denominator")

I did not have the courage to change this, so I implemented a hack in RationalFunctionField._element_constructor_that fixes this issue at least.


New commits:

831cfa6fix 29713

comment:2 Changed 17 months ago by git

  • Commit changed from 831cfa6b6273f1abf44ba13759b5edd51745cd3c to 056f0669b8072a3fe472c2cff1e1cd0ba5c3eba1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

056f066fix 29713

comment:3 Changed 17 months ago by git

  • Commit changed from 056f0669b8072a3fe472c2cff1e1cd0ba5c3eba1 to 3e1096fab63edca04e50aec4d97ce7270743ed4e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3e1096ffix 29713

comment:4 Changed 17 months ago by git

  • Commit changed from 3e1096fab63edca04e50aec4d97ce7270743ed4e to 7c197d13a3b6bdc595e0750f7fa927655e3c7a99

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7c197d1fix 29713

comment:5 Changed 17 months ago by tscrim

  • Status changed from needs_review to needs_work

I think this is a pretty bad hack and it doesn't solve the underlying problem:

sage: F = FractionField(QQ['a'])
sage: a = F.gen()
sage: R = PolynomialRing(F, 'x')
sage: FF = FractionField(R)
sage: elt = F(-1/2/(a^2+a))
sage: FF(elt)
-1/2/(a^2 + a)
sage: F(_)  # Same error

I think the correct fix would be to change something in the fraction field _element_constructor_ to handle this case.

comment:6 Changed 17 months ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch changed from u/heluani/29713 to public/rings/fraction_field_roundtrip-29713
  • Cc mmezzarobba caruso slelievre saraedum vdelecroix added
  • Commit changed from 7c197d13a3b6bdc595e0750f7fa927655e3c7a99 to 52e465147710e82874e3f5906f3e804e7959ce77
  • Status changed from needs_work to needs_review

Here is my proposal that should make sure such things work. It might not be the fastest, but there is some attempt at making the most natural code paths fast.


New commits:

52e4651Allowing a larger number of things to convert into the fraction field.

comment:7 Changed 17 months ago by caruso

I mostly agree with your proposal but I would have implemented things slightly differently.

comment:8 Changed 17 months ago by git

  • Commit changed from 52e465147710e82874e3f5906f3e804e7959ce77 to 7de03008dc6315bf93919e784697f112a493cd76

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

7de0300another proposal

comment:9 follow-up: Changed 17 months ago by caruso

Moreover, I'm not sure that having a special case when y is not none and parent(x) is self is really relevant. I agree that it can short-circuit something but, on the contrary, I think that conveting y to self may have some impact on the efficiency.

comment:10 in reply to: ↑ 9 Changed 17 months ago by tscrim

Replying to caruso:

Moreover, I'm not sure that having a special case when y is not none and parent(x) is self is really relevant. I agree that it can short-circuit something but, on the contrary, I think that conveting y to self may have some impact on the efficiency.

I somewhat agree. It was my first attempt at a solution, but I felt could cover some extra similar such cases. Why I convert y to self is this: If we try and short-circuit it this way, there can be some extra conversions that are there, but are not coercions. The first thing I tried was:

y *= x.denominator()

The other option would be to use

y *= parent(y)(x.denominator())

but I feel that could involve even more conversions. I also concluded that if we didn't short-circuit, there would be just as many conversions that would be done using the default pathway (plus extra exceptions raised).

comment:11 Changed 17 months ago by caruso

  • Reviewers set to Xavier Caruso

OK.

So, if you agree with my changes, you can give a positive review to this ticket on my behalf.

comment:12 Changed 17 months ago by tscrim

  • Milestone changed from sage-9.3 to sage-9.2
  • Status changed from needs_review to positive_review

Yes, I agree. Thank you for the review.

Addendum - I forgot to say, I like your fix by checking the parent.

Last edited 17 months ago by tscrim (previous) (diff)

comment:13 Changed 17 months ago by vbraun

  • Branch changed from public/rings/fraction_field_roundtrip-29713 to 7de03008dc6315bf93919e784697f112a493cd76
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.