Opened 13 years ago

Closed 12 years ago

Last modified 7 years ago

#7981 closed defect (fixed)

animate ignores options to show that are passed up from the plot command

Reported by: Jason Grout Owned by: Andrey Novoseltsev
Priority: major Milestone: sage-4.6.2
Component: graphics Keywords:
Cc: William Cauchois Merged in: sage-4.6.2.alpha2
Authors: Jason Grout, Andrey Novoseltsev Reviewers: Tim Dumol, Marshall Hampton, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Frédéric Chapoton)

This animate command shouldn't ignore the options to show() that are passed through by the plot command (here the options are ymin and ymax):

sage: var('t')
sage: damped_oscillator = 41/311*sqrt(311)*e^(-3/8*t)*sin(1/8*sqrt(311)*t) + 3*e^(-3/8*t)*cos(1/8*sqrt(311)*t)
sage: animate([plot( lambda x: damped_oscillator( t = x + k ), -1/2, 3*pi, ymin=-2, ymax=3.5 ) for k in srange( 0, pi, 0.3 ) ]).show()

Thanks to Johann Myrkraverk Oskarsson for reporting this.

Attachments (3)

trac-7981-show_options.patch (5.6 KB) - added by Jason Grout 13 years ago.
trac-7981-save_ignores_preset_plotting_options.patch (8.3 KB) - added by Andrey Novoseltsev 12 years ago.
Alternative patch
trac_7981-reviewer.patch (904 bytes) - added by Karl-Dieter Crisman 12 years ago.
reviewer patch

Download all attachments as: .zip

Change History (35)

Changed 13 years ago by Jason Grout

comment:1 Changed 13 years ago by Jason Grout

Status: newneeds_review

comment:2 Changed 13 years ago by Jason Grout

Authors: Jason Grout

comment:3 Changed 13 years ago by Jason Grout

Cc: William Cauchois Karl-Dieter Crisman added

comment:4 Changed 13 years ago by Jason Grout

Bill and Karl-Dieter,

You two might be interested in this. Karl-Dieter, you wrote the original code that passed show options around, I believe. I just made the consolidation happen in .save() instead of .show().

comment:5 Changed 13 years ago by Jason Grout

This fixes #7524.

comment:6 Changed 13 years ago by Karl-Dieter Crisman

Cc: Karl-Dieter Crisman removed

Umm... I find that unlikely, though I may have broken something inadvertently.

comment:7 Changed 13 years ago by Tim Dumol

Reviewers: Tim Dumol
Status: needs_reviewneeds_work

Doctesting on plot.py results in 2 errors:

{{[ sage -t "devel/sage-ref/sage/plot/plot.py"

--leak-resolution=high --log-socket=127.0.0.1 --leak-check=full --num-callers=25 --suppressions=/opt/sage-4.3.rc0.O0/local/lib/valgrind/sage.supp

File "/home/timdumol/sage-4.3.1.alpha0/devel/sage-ref/sage/plot/plot.py", line 1925:

sage: c.show(figsize=[5,5], xmin=-1, xmax=3, ymin=-1, ymax=3) sage: point((-1,1),pointsize=30, color='red')

Exception raised:

Traceback (most recent call last):

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test

self.run_one_example(test, example, filename, compileflags)

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example

OrigDocTestRunner?.run_one_example(self, test, example, filename, compileflags)

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example

compileflags, 1) in test.globs

File "<doctest main.example_37[5]>", line 1

c.show(figsize=[Integer(5),Integer(5)], xmin=-Integer(1), xmax=Integer(3), ymin=-Integer(1), ymax=Integer(3)) sage: point((-Integer(1),Integer(1)),pointsize=Integer(30), color='red')###line 1925:

sage: c.show(figsize=[5,5], xmin=-1, xmax=3, ymin=-1, ymax=3) sage: point((-1,1),pointsize=30, color='red')

SyntaxError?: invalid syntax

File "/home/timdumol/sage-4.3.1.alpha0/devel/sage-ref/sage/plot/plot.py", line 1930:

sage: c.save(DOCTEST_MODE_FILE)

Exception raised:

Traceback (most recent call last):

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test

self.run_one_example(test, example, filename, compileflags)

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example

OrigDocTestRunner?.run_one_example(self, test, example, filename, compileflags)

File "/home/timdumol/sage-4.3.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example

compileflags, 1) in test.globs

File "<doctest main.example_37[7]>", line 1, in <module>

c.save(DOCTEST_MODE_FILE)###line 1930:

sage: c.save(DOCTEST_MODE_FILE) NameError?: name 'DOCTEST_MODE_FILE' is not defined

1 items had failures:

2 of 8 in main.example_37

}}}

comment:8 Changed 12 years ago by Jason Grout

Here is another effect of this, I think:

plot(x,(x,0,1),aspect_ratio=1).save('test.png')

does not save a graph with aspect ratio 1.

comment:9 Changed 12 years ago by Andrey Novoseltsev

Authors: Jason GroutJason Grout, Andrey Novoseltsev
Status: needs_workneeds_review

I have just wrote a patch fixing the issue for save (which is quite annoying in conjunction with SageTeX) and then found this ticket. Since the original patch does not apply cleanly on sage-4.6.alpha1, I posted my patch, but I have incorporated Jason's changes to show to eliminate double processing of options.

I have changed call parameters to save since it is not documented why one would ever need savenow=False. If I want to save later, shouldn't I call save later?-) Also it does not make sense in my opinion to use any extra positional arguments in this function except for the file name. I realize that this is a backward-incompatible change, but all long tests pass with this patch.

comment:10 Changed 12 years ago by Andrey Novoseltsev

Made it possible to apply the new patch after #10291 (which is now positively reviewed).

comment:11 Changed 12 years ago by Andrey Novoseltsev

Owner: changed from William Stein to Andrey Novoseltsev

For the buildbot

Apply trac-7981-save_ignores_preset_plotting_options.patch

comment:12 Changed 12 years ago by mhampton

Reviewers: Tim DumolTim Dumol, Marshall Hampton
Status: needs_reviewpositive_review

This seems to work, and all doctests in the module plot (not just the file) pass.

Sadly it didn't also fix #10244, so I will try to figure that out if I can.

Positive review.

comment:13 Changed 12 years ago by Karl-Dieter Crisman

Reviewers: Tim Dumol, Marshall HamptonTim Dumol, Marshall Hampton, Karl-Dieter Crisman
Status: positive_reviewneeds_work

Just out of curiosity, what is the 'backward-incompatible' change you mention? Which extra positional arguments - like dpi? (Though Jason also got rid of that - I wonder why?)

I guess I mean to ask whether this is a good such change; usually there is a deprecation period. After all, doctests catch very few of our use cases :) What is wrong with the usual *args,**kwds syntax?

As for savenow, it looks like with it being False we could still create a Sage object. You are right that it seems a little redundant, though!

Also, this needs a doctest (it's in the original patch) to show that animate options actually work, at least in theory (if one looked at it and ran the optional tests). So... needs work. Sorry :(

comment:14 in reply to:  13 ; Changed 12 years ago by Andrey Novoseltsev

Status: needs_workneeds_review

Replying to kcrisman:

Just out of curiosity, what is the 'backward-incompatible' change you mention? Which extra positional arguments - like dpi? (Though Jason also got rid of that - I wonder why?)

I don't remember exactly what I meant, but probably it was changing parameters of save.

I guess I mean to ask whether this is a good such change; usually there is a deprecation period. After all, doctests catch very few of our use cases :) What is wrong with the usual *args,**kwds syntax?

I think that it makes the syntax of save cleaner and easier to understand (and document for that matter). As was recently mentioned on sage-devel, one should use common sense when deciding whether to deprecate something or change immediately, I think that these changes fall into the latter category ;-) As for *args I just think that it is a bad practice to call functions with 20 or so possible parameters listing them without names.

As for savenow, it looks like with it being False we could still create a Sage object. You are right that it seems a little redundant, though!

Isn't it a bug that save saves something in some cases when savenow=False?..

Also, this needs a doctest (it's in the original patch) to show that animate options actually work, at least in theory (if one looked at it and ran the optional tests). So... needs work. Sorry :(

I added the doctest. Was it the only reason for "needs work" or you would like to have save parameters changed as well?

Changed 12 years ago by Andrey Novoseltsev

Alternative patch

comment:15 Changed 12 years ago by Andrey Novoseltsev

For the confused buildbot:

Apply trac-7981-save_ignores_preset_plotting_options.patch

comment:16 in reply to:  14 Changed 12 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_review

Replying to novoselt:

Replying to kcrisman: I think that it makes the syntax of save cleaner and easier to understand (and document for that matter). As was recently mentioned on sage-devel, one should use common sense when deciding whether to deprecate something or change immediately, I think that these changes fall into the latter category ;-)

Yeah, these two make sense. It looks like dpi will still work, given SHOW_OPTIONS.

As for *args I just think that it is a bad practice to call functions with 20 or so possible parameters listing them without names.

Okay, I see what's going on here now. Especially since the order would be open to suspicion!

As for savenow, it looks like with it being False we could still create a Sage object. You are right that it seems a little redundant, though!

Isn't it a bug that save saves something in some cases when savenow=False?..

No, just an undocumented feature! Since it doesn't save a graphic. I agree this seems odd, though, so not complaining.

Also, this needs a doctest (it's in the original patch) to show that animate options actually work, at least in theory (if one looked at it and ran the optional tests). So... needs work. Sorry :(

I added the doctest. Was it the only reason for "needs work" or you would like to have save parameters changed as well?

No, assuming this still applies by the buildbot, and since you've explained the parameter issue fine, that's okay. The only reason I felt justified in overruling the original positive review was because it didn't actually include the doctest, though I had the other questions as well.

comment:17 Changed 12 years ago by Jeroen Demeyer

Milestone: sage-4.6.1sage-4.6.2

comment:18 Changed 12 years ago by Karl-Dieter Crisman

There is a trivial error in this patch that however causes it to fail doctests.

We check that Trac #7981 is fixed:: 

    sage: animate([plot(sin(x + float(k), (0, 2*pi), ymin=-5, ymax=5)) 
    ...            for k in srange(0,2*pi,0.3)]).show() # optional

Notice that there is a parenthesis missing after float(k) which instead comes after ymax=5. Patch coming up.

comment:19 Changed 12 years ago by Karl-Dieter Crisman

Okay, now for sure positive review.

To buildbot - apply trac-7981-save_ignores_preset_plotting_options.patch and trac_7981-reviewer.patch.

comment:20 Changed 12 years ago by Karl-Dieter Crisman

Just FYI - still applies fine on 4.6.2.alpha0.

Changed 12 years ago by Karl-Dieter Crisman

Attachment: trac_7981-reviewer.patch added

reviewer patch

comment:21 Changed 12 years ago by Karl-Dieter Crisman

Just an update - apparently

sage: animate([plot(sin(x + float(k)), (0, 2*pi), ymin=-5, ymax=5)
...            for k in srange(0,2*pi,0.3)]).show() # optional

does not obey the optional test, for it created a new file (I must have ImageMagick?!). We don't create non-temp new files in doctests, though, so this had to be changed. New reviewer patch fixes this as well, maintains positive review. Should not affect the plot patches which depend on this.

comment:22 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

Maybe you should document in the file what you just said in your comment. Also make it clear why the test is tagged optional.

comment:23 Changed 12 years ago by Karl-Dieter Crisman

Status: needs_workneeds_review

Hmm, but in many places in this file it says why such things are optional. In fact, earlier in the same docstring the first occurrence of .show() explains this:

        sage: a.show()          # optional -- requires convert command

as well as several lines later

        sage: a.show() # optional -- requires convert command

so hopefully one wouldn't need to do it a third time in three paragraphs. Especially since it's a TEST.

Also, the actual issue with creating a file I have posted to sage-devel about; it's not 100% clear to me that this is a bug. It just happened to have a bad effect here, which I changed from Andrey's patch. But it's orthogonal to the ticket.

So putting back to 'needs review'. I hope you will agree with me that this is in fact still worthy of positive review.

Now, of course there is in the doctesting framework the issue that one can do optional tests with only certain keywords, so if one has convert one could run them with that keyword. But in that case, the entire file animate.py is replete with violations of this, and I feel that should be a separate ticket.

comment:24 in reply to:  23 ; Changed 12 years ago by Jeroen Demeyer

Replying to kcrisman:

Hmm, but in many places in this file it says why such things are optional. In fact, earlier in the same docstring the first occurrence of .show() explains this:

        sage: a.show()          # optional -- requires convert command

as well as several lines later

        sage: a.show() # optional -- requires convert command

so hopefully one wouldn't need to do it a third time in three paragraphs. Especially since it's a TEST.

Personally, I would write it a third time. On the other hand, I don't care too much. So if you feel like you're happy with the patch as-is, then that's fine for me.

comment:25 in reply to:  24 Changed 12 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_review

so hopefully one wouldn't need to do it a third time in three paragraphs. Especially since it's a TEST.

Personally, I would write it a third time. On the other hand, I don't care too much. So if you feel like you're happy with the patch as-is, then that's fine for me.

Yes, I am. This issue is pretty important, and the other issue is somewhat orthogonal. I've opened another ticket for the issue about the optional keyword - this is now #10655.

comment:26 Changed 12 years ago by Karl-Dieter Crisman

'This issue' meaning this ticket itself, of course :-)

comment:27 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_info

Please specify which patches have to be applied.

comment:28 Changed 12 years ago by Andrey Novoseltsev

Status: needs_infoneeds_review

Please apply trac-7981-save_ignores_preset_plotting_options.patch and trac_7981-reviewer.patch

comment:29 Changed 12 years ago by Andrey Novoseltsev

Status: needs_reviewpositive_review

comment:30 in reply to:  27 Changed 12 years ago by Karl-Dieter Crisman

Replying to jdemeyer:

Please specify which patches have to be applied.

Just FYI, this was already noted in comment 19.

comment:31 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.alpha2
Resolution: fixed
Status: positive_reviewclosed

comment:32 Changed 7 years ago by Frédéric Chapoton

Description: modified (diff)
Note: See TracTickets for help on using tickets.