"fraction" fixes - #4462
Conversation
d-torrance
left a comment
d-torrance
left a comment
There was a problem hiding this comment.
Reviewed with @local-ring. Just a couple questions and comments
| gens gb Iloc | ||
| /// | ||
|
|
||
| TEST /// -- normalize ambient-unit denominators |
There was a problem hiding this comment.
This test never gets run because of the end on line 875. Would you move it up?
(While you're at it, would you mind moving up the promote/lift test just underneath it? I fell into the same trap in #4204 lol.)
| // TODO uniformise behaviour of invert for noninvertible elements | ||
| if (dynamic_cast<const PolyRingQuotient *>(R_) != nullptr) | ||
| { | ||
| denom_inverse = R_->invert(f->denom); // for quotient rings, don't call is_unit since it calls invert internally |
There was a problem hiding this comment.
With this, we can run into #3973, where we didn't before:
i1 : K = toField(QQ[a]/(a^2 - 2))
o1 = K
o1 : PolynomialRing
i2 : B = K[t]/(t^2 - a)
o2 = B
o2 : QuotientRing
i3 : 1/(t + 1)
--internal error: ring element gcd computation failed
Aborted (core dumped)In 1.26.06, we get:
i3 : 1/(t + 1)
1
o3 = -----
t + 1
o3 : frac B| const RingElement *b = RingElement::make_raw(R_, y); | ||
| const RingElement *c = rawGCDRingElement(a, b, nullptr, false); | ||
| if (!c) return; | ||
| if (!R_->is_equal(y, R_->one())) |
There was a problem hiding this comment.
If simplify_unit_denominator(f) would have returned true, so this if statement isn't necessary. (And the same comment for the local ring version below.)
|
|
||
| ring_elem denom_inverse; | ||
| // TODO uniformise behaviour of invert for noninvertible elements | ||
| if (dynamic_cast<const PolyRingQuotient *>(R_) != nullptr) |
There was a problem hiding this comment.
I think R_->is_quotient_ring() would be clearer
| if (dynamic_cast<const PolyRingQuotient *>(R_) != nullptr) | ||
| { | ||
| denom_inverse = R_->invert(f->denom); // for quotient rings, don't call is_unit since it calls invert internally | ||
| if (R_->is_zero(denom_inverse)) return false; // for non invertible elements, returns zero denominator |
There was a problem hiding this comment.
What does "returns zero denominator" mean in this comment?
2 participants
This fixes two somewhat related bugs in the handling of fractions:
basically, M2 did not correctly simplify the denominator when it was a unit, leading to lots of oddities (this is only the simplest example). now it does a much cleaner job.
AI disclosure
I formulated the fixes, but codex helped with the c++.