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

Add doctest that integral of 0 function is 0 #39362

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

DaveWitteMorris
Copy link
Member

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

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

Documentation preview for this PR (built with commit e1e4071; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor

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.

@DaveWitteMorris
Copy link
Member Author

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 make ptestlong does, then I think there is a stronger argument that the test is redundant.

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.

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.

Make sure integral(f(x) - f(x).expand(), (x, 0, 2*pi)) is zero
2 participants