-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Series: Removed incosistency in order in case of Zero #18629
Conversation
Added tests
✅ Hi, I am the SymPy bot (v149). 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.6. 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 #18629 +/- ##
==============================================
- Coverage 75.586% 64.348% -11.239%
==============================================
Files 644 644
Lines 167464 167515 +51
Branches 39466 39485 +19
==============================================
- Hits 126581 107794 -18787
- Misses 35369 53552 +18183
- Partials 5514 6169 +655 |
sympy/series/tests/test_order.py
Outdated
@@ -435,3 +435,6 @@ def test_issue_15539(): | |||
assert O(1/x**4 + exp(x), (x, -oo)) == O(1/x**4, (x, -oo)) | |||
assert O(1/x**4 + exp(-x), (x, -oo)) == O(exp(-x), (x, -oo)) | |||
assert O(1/x, (x, oo)).subs(x, -x) == O(-1/x, (x, -oo)) | |||
|
|||
def test_issue_18606(): | |||
assert Order(0) == O(0) |
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.
You need to use the unchanged
function here. Otherwise this test will always pass, even the way it was before.
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.
Done.
I'm glad to see this doesn't require many changes. I have just the two comments and it looks fine otherwise. |
sympy/core/add.py
Outdated
# 2 + O(0) -> 2 | ||
for o in order_factors: | ||
if o.expr.is_zero: | ||
order_factors.remove(o) |
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.
You shouldn't modify a list while looping through it in Python.
Can this loop be merged with the one above?
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.
No, it can't be merged with the loop above because if newseq is empty it doesn't iterate through the loop.
Btw thanks for pointing out the fact one shouldn't modify a list while looping through it.
sympy/core/add.py
Outdated
# 2 + O(0) -> 2 | ||
new_order_factors = [] | ||
for o in order_factors: | ||
if not o.expr.is_zero: |
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 would have tried to handle this in another way: not adding O(0)
to order_factors
at all. So the first thing after this line
Line 125 in 0084405
if o.is_Order: |
could be
if o.expr.is_zero:
continue
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.
This is a better way of doing it. Thanks @jksuom
Thanks, this looks good. |
Added tests
References to other Issues or PRs
Fixes #13715, #18606
Brief description of what is fixed or changed
Order(0) now returns an object of Order Type
Other comments
Release Notes