Opened 8 years ago

Closed 6 years ago

#18212 closed defect (fixed)

fix NameError in mathematica_free integration

Reported by: github/bukzor Owned by:
Priority: major Milestone: sage-8.0
Component: interfaces: optional Keywords:
Cc: Merged in:
Authors: Buck Evan, Karl-Dieter Crisman Reviewers: Karl-Dieter Crisman, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 5d3207a (Commits, GitHub, GitLab) Commit: 5d3207aad5e434be48f7dfd4c3db5cbd4c1b000b
Dependencies: Stopgaps:

Status badges

Description

It seems like the purpose was to normalize the variable of integration to x.

The line in question substitutes x for dvar rather than the variable of integration passed in, v.

https://github.com/sagemath/sage/pull/37

Change History (22)

comment:1 Changed 8 years ago by github

Branch: u/github/ticket/18212
Status: newneeds_review

comment:2 Changed 8 years ago by git

Commit: 1b5e93620d3ac86ea9601db270c0f87548ae36e0

Branch pushed to git repo; I updated commit sha1. New commits:

1b5e936fix NameError in mathematica_free integration

comment:3 Changed 8 years ago by kcrisman

Component: PLEASE CHANGEinterfaces
Type: PLEASE CHANGEdefect

comment:4 Changed 8 years ago by kcrisman

Authors: Buck Evan

comment:5 Changed 8 years ago by kcrisman

Can you give an example of where this NameError occurs? It would make little sense to change this without somehow doctesting what it was that went wrong. In the most recent Sage devel version I get

sage: integral(sin(x)^2, x, algorithm='mathematica_free')
-1/2*cos(x)*sin(x) + 1/2*x

comment:6 Changed 8 years ago by kcrisman

Status: needs_reviewneeds_work
Work issues: example needed, doctest

comment:7 Changed 8 years ago by github

github/kcrisman wrote:

Hi! Let's continue the discussion at http://trac.sagemath.org/ticket/18212.

comment:8 in reply to:  5 Changed 8 years ago by kcrisman

Can you give an example of where this NameError occurs? It would make little sense to change this without somehow doctesting what it was that went wrong. In the most recent Sage devel version I get

Figured it out.

sage: var('y')
y
sage: integral(sin(y)^2, y, algorithm='mathematica_free')
---------------------------------------------------------------------------
NameError: global name 'dvar' is not defined

comment:9 Changed 8 years ago by kcrisman

Authors: Buck EvanBuck Evan, Karl-Dieter Crisman
Branch: u/github/ticket/18212u/kcrisman/ticket/18212
Commit: 1b5e93620d3ac86ea9601db270c0f87548ae36e05078cf84761975ad3539bef966655ef328f1e0f5
Reviewers: Karl-Dieter Crisman
Status: needs_workneeds_review
Work issues: example needed, doctest

I'm pushing a branch with doctests, and in doing this made a few more optional for internet so one doesn't have the optional=internet,sage problem as usual, then I replicated a different test elsewhere so it still gets tested ordinarily. Basically just needs review that this all works.


New commits:

c254015Merge branch 'u/github/ticket/18212' of git://trac.sagemath.org/sage into ticket/18212
5078cf8Doctest Trac 18212 for mma_free integration fix

comment:10 Changed 8 years ago by jdemeyer

Status: needs_reviewneeds_info

Why are these tests marked # optional?

sage: _ = var('x, y, z') # optional - internet
sage: f = sin(x^2) + y^z # optional - internet

comment:11 Changed 8 years ago by jdemeyer

Same question here:

sage: from sage.symbolic.integration.external import mma_free_integrator # optional - internet

If importing the module requires internet, you're doing it wrong...

comment:12 Changed 8 years ago by jdemeyer

And there is this oddity:

Test a few imports, without internet::

???

comment:13 in reply to:  9 Changed 8 years ago by kcrisman

I'm pushing a branch with doctests, and in doing this made a few more optional for internet so one doesn't have the optional=internet,sage problem as usual, then I replicated a different test elsewhere so it still gets tested ordinarily.

I hope that this quote from comment:9 answers your questions. It's very annoying to do --optional=internet and then have tests fail because the "prereq" lines weren't evaluated. So that is my philosophy, to avoid --optional=sage,internet which I view as more than annoying to remember.

comment:14 Changed 8 years ago by jdemeyer

Ah, I see now.

comment:15 Changed 8 years ago by jdemeyer

If you really have problems with the fact that ./sage -t --optional=internet tests do not work, the proper fix would simply be to change the doctest framework (after sending a message to sage-devel of course). As far as I know, when we rewrote the doctest framework in #12415, we implemented --optional the way it is simply to emulate historical behaviour, I don't think it was a conscious choice.

comment:16 Changed 8 years ago by jdemeyer

The fact that your patch actually needs to duplicate a doctest (once with # optional and once without) shows to me that your solution is not good.

comment:17 Changed 8 years ago by kcrisman

Well, I only do that so that it's 100% sure to be doctested. Submit a change you like and I'll review it, this isn't that horrible in any case, the key is to fix the error.

comment:18 Changed 8 years ago by jdemeyer

Component: interfacesinterfaces: optional

comment:19 Changed 6 years ago by chapoton

Branch: u/kcrisman/ticket/18212public/18212
Commit: 5078cf84761975ad3539bef966655ef328f1e0f55d3207aad5e434be48f7dfd4c3db5cbd4c1b000b

New commits:

5d3207aMerge branch 'u/kcrisman/ticket/18212' in 8.0.b5

comment:20 Changed 6 years ago by chapoton

Milestone: sage-6.7sage-8.0
Status: needs_infoneeds_review

comment:21 Changed 6 years ago by tscrim

Reviewers: Karl-Dieter CrismanKarl-Dieter Crisman, Travis Scrimshaw
Status: needs_reviewpositive_review

While I agree with Jeroen that we should have the minimal amount of # optional tags as needed, I see this as essentially bikeshedding. So I'm setting this to a positive review.

comment:22 Changed 6 years ago by vbraun

Branch: public/182125d3207aad5e434be48f7dfd4c3db5cbd4c1b000b
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.