Opened 17 months ago
Closed 17 months ago
#29713 closed defect (fixed)
Broken conversion from FractionField over PolynomialRing over Field back to Field
Reported by:  ghpetRUShka  Owned by:  

Priority:  major  Milestone:  sage9.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: 
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/sitepackages/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: 20200101
Change History (13)
comment:1 Changed 17 months ago by
 Branch set to u/heluani/29713
 Commit set to 831cfa6b6273f1abf44ba13759b5edd51745cd3c
 Status changed from new to needs_review
comment:2 Changed 17 months ago by
 Commit changed from 831cfa6b6273f1abf44ba13759b5edd51745cd3c to 056f0669b8072a3fe472c2cff1e1cd0ba5c3eba1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
056f066  fix 29713

comment:3 Changed 17 months ago by
 Commit changed from 056f0669b8072a3fe472c2cff1e1cd0ba5c3eba1 to 3e1096fab63edca04e50aec4d97ce7270743ed4e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3e1096f  fix 29713

comment:4 Changed 17 months ago by
 Commit changed from 3e1096fab63edca04e50aec4d97ce7270743ed4e to 7c197d13a3b6bdc595e0750f7fa927655e3c7a99
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7c197d1  fix 29713

comment:5 Changed 17 months ago by
 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
 Branch changed from u/heluani/29713 to public/rings/fraction_field_roundtrip29713
 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:
52e4651  Allowing a larger number of things to convert into the fraction field.

comment:7 Changed 17 months ago by
I mostly agree with your proposal but I would have implemented things slightly differently.
comment:8 Changed 17 months ago by
 Commit changed from 52e465147710e82874e3f5906f3e804e7959ce77 to 7de03008dc6315bf93919e784697f112a493cd76
Branch pushed to git repo; I updated commit sha1. New commits:
7de0300  another proposal

comment:9 followup: ↓ 10 Changed 17 months ago by
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 shortcircuit 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
Replying to caruso:
Moreover, I'm not sure that having a special case when
y
is not none andparent(x)
isself
is really relevant. I agree that it can shortcircuit something but, on the contrary, I think that convetingy
toself
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 shortcircuit 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 shortcircuit, 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
 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
 Milestone changed from sage9.3 to sage9.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.
comment:13 Changed 17 months ago by
 Branch changed from public/rings/fraction_field_roundtrip29713 to 7de03008dc6315bf93919e784697f112a493cd76
 Resolution set to fixed
 Status changed from positive_review to closed
I feel like the right solution is to have FractionFieldEmbeddingSection?._call_ raise a
ValueError
instead of aTypeError
. From the code in fraction_field.py: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:
fix 29713