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:  sage8.0 
Component:  interfaces: optional  Keywords:  
Cc:  Merged in:  
Authors:  Buck Evan, KarlDieter Crisman  Reviewers:  KarlDieter Crisman, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  5d3207a (Commits, GitHub, GitLab)  Commit:  5d3207aad5e434be48f7dfd4c3db5cbd4c1b000b 
Dependencies:  Stopgaps: 
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
.
Change History (22)
comment:1 Changed 8 years ago by
Branch:  → u/github/ticket/18212 

Status:  new → needs_review 
comment:2 Changed 8 years ago by
Commit:  → 1b5e93620d3ac86ea9601db270c0f87548ae36e0 

comment:3 Changed 8 years ago by
Component:  PLEASE CHANGE → interfaces 

Type:  PLEASE CHANGE → defect 
comment:4 Changed 8 years ago by
Authors:  → Buck Evan 

comment:5 followup: 8 Changed 8 years ago by
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
Status:  needs_review → needs_work 

Work issues:  → example needed, doctest 
comment:7 Changed 8 years ago by
github/kcrisman wrote:
Hi! Let's continue the discussion at http://trac.sagemath.org/ticket/18212.
comment:8 Changed 8 years ago by
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 followup: 13 Changed 8 years ago by
Authors:  Buck Evan → Buck Evan, KarlDieter Crisman 

Branch:  u/github/ticket/18212 → u/kcrisman/ticket/18212 
Commit:  1b5e93620d3ac86ea9601db270c0f87548ae36e0 → 5078cf84761975ad3539bef966655ef328f1e0f5 
Reviewers:  → KarlDieter Crisman 
Status:  needs_work → needs_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:
c254015  Merge branch 'u/github/ticket/18212' of git://trac.sagemath.org/sage into ticket/18212

5078cf8  Doctest Trac 18212 for mma_free integration fix

comment:10 Changed 8 years ago by
Status:  needs_review → needs_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
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
And there is this oddity:
Test a few imports, without internet::
???
comment:13 Changed 8 years ago by
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:15 Changed 8 years ago by
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 sagedevel 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
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
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
Component:  interfaces → interfaces: optional 

comment:19 Changed 6 years ago by
Branch:  u/kcrisman/ticket/18212 → public/18212 

Commit:  5078cf84761975ad3539bef966655ef328f1e0f5 → 5d3207aad5e434be48f7dfd4c3db5cbd4c1b000b 
New commits:
5d3207a  Merge branch 'u/kcrisman/ticket/18212' in 8.0.b5

comment:20 Changed 6 years ago by
Milestone:  sage6.7 → sage8.0 

Status:  needs_info → needs_review 
comment:21 Changed 6 years ago by
Reviewers:  KarlDieter Crisman → KarlDieter Crisman, Travis Scrimshaw 

Status:  needs_review → positive_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
Branch:  public/18212 → 5d3207aad5e434be48f7dfd4c3db5cbd4c1b000b 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
fix NameError in mathematica_free integration