Add doctest that integral of 0 function is 0 #39362
Conversation
|
Documentation preview for this PR (built with commit e1e4071; changes) is ready! 🎉 |
|
Forgive me for being annoying. This wasn't a bug anywhere in sage, and the maxima commit that fixed this issue added several tests for it: https://sourceforge.net/p/maxima/code/ci/d243bcea6ba6a72623ad0fad21e681a5f9696c5e If you are running the tests for maxima, the new test in sage is redundant and will just waste a few milliseconds. Our dependencies are fixing thousands of bugs each year -- many of which manifest in sage if you are clever enough -- and I think as a guiding principle, it is counterproductive to duplicate upstream tests (when upstream writes tests!) in sage. Our test suite is slow enough already and should focus on testing the code that we are responsible for. Now with that out of the way... this one example is small and pretty harmless. So if you still want to add it, OK. |
|
I've never heard this before. I would say that sage had a bug, and it has been fixed. However, it's true that the bug was fixed upstream. When we were on trac, there were specific flags to track the upstream progress. When upstream fixed the bug, and it got into a release that sage used, I think it was common to add a doctest, under the principle that every reported sage bug gets a doctest when it is fixed. (I certainly saw this happen several times, but maybe I had a skewed perspective and adding a doctest actually wasn't the norm.) As far as I know, I never run the maxima test suite (or the test suite for gap or other major dependencies). On the other hand, if I don't know which way is better, but I thought sage practice was to doctest these upstream issues before closing them. A clear policy would be good. |
|
On 2025-01-26 10:14:05, DaveWitteMorris wrote:
When we were on trac, there were specific flags to track the upstream progress. When upstream fixed the bug, and it got into a release that sage used, I think it was common to add a doctest, under the principle that every reported sage bug gets a doctest when it is fixed. (I certainly saw this happen several times, but maybe I had a skewed perspective and adding a doctest actually wasn't the norm.)
No, you're right, it's an old practice. It made more sense when all of Sage's dependencies were bundled and highly patched, often with bugfixes coming _from_ patches. But today, everyone should be using the maxima that their linux distro (or conda, or homebrew, etc) provides. If the goal is to prevent the bug from coming back, and if everyone is using the upstream package, then it suffices for maxima to have a test for it upstream.
As far as I know, I never run the maxima test suite (or the test suite for gap or other major dependencies). On the other hand, if `make ptestlong` does, then I think there is a stronger argument that the test is redundant.
The sage distro will run these tests if you set SAGE_CHECK=yes, and some of the Github CI jobs run them. Linux distro packagers run them as well before creating each package. So it's not _everyone_, but enough people are running them that the bug isn't going to sneak back in unnoticed.
I don't know which way is better, but I thought sage practice was to doctest these upstream issues before closing them. A clear policy would be good.
Easier to just let this one go :)
|
|
OK, what you say makes sense. In the future, I won't add a doctest when the fix was upstream, unless I have some special reason for it. PS Thanks for the review and comments! |
Fixes sagemath#33034. Issue sagemath#33034 pointed out that integrating a function that is identically zero can produce a nonzero value. However, this was a maxima bug that was fixed in sage 10.1. So we just add a doctest. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39362 Reported by: DaveWitteMorris Reviewer(s):
Fixes sagemath#33034. Issue sagemath#33034 pointed out that integrating a function that is identically zero can produce a nonzero value. However, this was a maxima bug that was fixed in sage 10.1. So we just add a doctest. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39362 Reported by: DaveWitteMorris Reviewer(s):
Fixes sagemath#33034. Issue sagemath#33034 pointed out that integrating a function that is identically zero can produce a nonzero value. However, this was a maxima bug that was fixed in sage 10.1. So we just add a doctest. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39362 Reported by: DaveWitteMorris Reviewer(s):
Fixes sagemath#33034. Issue sagemath#33034 pointed out that integrating a function that is identically zero can produce a nonzero value. However, this was a maxima bug that was fixed in sage 10.1. So we just add a doctest. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39362 Reported by: DaveWitteMorris Reviewer(s):
Fixes #33034.
Issue #33034 pointed out that integrating a function that is identically zero can produce a nonzero value. However, this was a maxima bug that was fixed in sage 10.1. So we just add a doctest.
📝 Checklist
⌛ Dependencies