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

[Functions]: Fixes AttributeError in limit evaluation #19604

Merged
merged 2 commits into from Jun 20, 2020

Conversation

sachin-4099
Copy link
Contributor

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

Addresses #19586 for the 1.6 branch (for 1.6.1)

Other Comments

Regression Test has been added.

Release Notes

  • functions
    • Fixes AttributeError in limit evaluation

@sympy-bot
Copy link

sympy-bot commented Jun 20, 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.

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.1.

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.

Addresses #19586 for the 1.6 branch (for 1.6.1)

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

#### Release Notes


<!-- BEGIN RELEASE NOTES -->
* functions
  * Fixes `AttributeError` in limit evaluation
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@sachin-4099 sachin-4099 requested a review from oscarbenjamin Jun 20, 2020
@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 20, 2020

Thanks. Could you add a test for this?

Also is there a test for this on master? If not then one should be added there before the issue is closed.

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jun 20, 2020

Actually I was of the opinion that at both places no test is required. And that's why I didn't add one here as well.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 20, 2020

Actually I was of the opinion that at both places no test is required. And that's why I didn't add one here as well.

The normal reason to add a test is to ensure that future changes will not lead to a similar issue and that this particular example will always give the expected result in future. Also you need to bear in mind that one reason the test might fail in future is because of changes in an unrelated part of the codebase. It's impossible for any one contributor to understand all of the possible effects of many of the changes that are made in sympy. For that reason it is good to have a continuously expanding set of test results especially where there is some case that has clearly been overlooked before.

In this particular example it's easy to see that your fix is correct. However the fact that this problem was previously unnoticed indicates that this particular codepath is untested. It's important to try to ensure that every line of code runs at some point during the tests so that e.g. systemic changes in the core can be accompanied with fixes across the codebase.

I don't necessarily disagree with you but why is it that you think no test is required?

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jun 20, 2020

Alright I will add the test at both places.

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #19604 into 1.6 will increase coverage by 0.014%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##               1.6    #19604       +/-   ##
=============================================
+ Coverage   75.650%   75.665%   +0.014%     
=============================================
  Files          651       651               
  Lines       169552    169552               
  Branches     40023     40023               
=============================================
+ Hits        128267    128292       +25     
+ Misses       35669     35651       -18     
+ Partials      5616      5609        -7     

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 20, 2020

Again Github has not updated to reflect the fact that the Travis tests are complete even though it has registered the results from codecov (#19589).

I'm hoping that this comment will cause it to update.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jun 20, 2020

Actually it seems that the tests since the last commit have not finished. The link to Travis from the commit is pointing to the wrong build. The normal "details" link is missing

@oscarbenjamin oscarbenjamin merged commit eef1cfd into sympy:1.6 Jun 20, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants