Skip to content
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

Core: Fixes a minor performance issue #19537

Merged
merged 1 commit into from Jun 12, 2020
Merged

Conversation

sachin-4099
Copy link
Contributor

@sachin-4099 sachin-4099 commented Jun 11, 2020

Fixes: #19534

Other Comments

Regression Test has been added.

Release Notes

NO ENTRY

@sympy-bot
Copy link

sympy-bot commented Jun 11, 2020

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.

  • No release notes entry will be added for this pull request.

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.

Fixes: #19534 


#### Other Comments
Regression Test has been added.


#### Release Notes


<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #19537 into master will decrease coverage by 8.040%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19537       +/-   ##
=============================================
- Coverage   75.665%   67.624%   -8.041%     
=============================================
  Files          654       654               
  Lines       169900    169939       +39     
  Branches     40055     40065       +10     
=============================================
- Hits        128556    114921    -13635     
- Misses       35727     49388    +13661     
- Partials      5617      5630       +13     

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 12, 2020

@zachetienne can you test out this PR to see if it fixes the problem?

@zachetienne
Copy link
Contributor

zachetienne commented Jun 12, 2020

@zachetienne can you test out this PR to see if it fixes the problem?

Yes, I tried it out while the PR was undergoing the automatic checks. Indeed it fixes the problem, restoring the original performance.

@jksuom jksuom merged commit a3da22a into sympy:master Jun 12, 2020
3 checks passed
@zachetienne
Copy link
Contributor

zachetienne commented Jun 12, 2020

Thanks so much @sachin-4099 and @jksuom !

@asmeurer
Copy link
Member

asmeurer commented Jun 12, 2020

Perhaps we should also add something to the benchmarks, if there isn't already.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 12, 2020

After a significant regression we should add a benchmark to the benchmarks suite:
https://github.com/sympy/sympy_benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Major (127x) performance regression: series() on relatively simple (polynomial) expression
6 participants