-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix GeneratorsError for creating FractionField element with domain of PolynomialRing or FractionField #19713
Conversation
✅ Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Codecov Report
@@ Coverage Diff @@
## master #19713 +/- ##
=============================================
- Coverage 75.693% 75.692% -0.002%
=============================================
Files 658 664 +6
Lines 171307 172382 +1075
Branches 40441 40657 +216
=============================================
+ Hits 129669 130480 +811
- Misses 35973 36191 +218
- Partials 5665 5711 +46 |
Looks good to me |
sympy/polys/tests/test_fields.py
Outdated
F1 = ZZ.frac_field(a, b) | ||
F2 = F1.frac_field(x) | ||
numer = F2(a + b).numer | ||
numer == F1.poly_ring(x)(a + b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be assert
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll fix them
if isinstance(self.domain, PolynomialRing) and \ | ||
numer.ring == self.domain.ring: | ||
numer = self.ring.ground_new(numer) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could be done if self.domain
is a FractionField
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed them
…olyElement or self.domain is PolynomialRing and element is given as FracElement
Thanks, looks good. |
References to other Issues or PRs
Fixes #19581
Brief description of what is fixed or changed
Fraction field can have domain of fraction field or polynomial ring
and sometimes the coefficient itself can be the element from its domain.
This fixes two issues for constructing
ZZ.frac_field(a, b).frac_field(x)(a+b)
ZZ.poly_ring(a, b).frac_field(x)(a+b)
Other comments
Release Notes
GeneratorsError
for creating some elements ofFractionField
when its ground domain isFractionField
orPolynomialRing
.