Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8632 closed defect (fixed)

.save ignores ymin etc.

Reported by: damm Owned by: was
Priority: major Milestone: sage-4.6.2
Component: graphics Keywords:
Cc: kcrisman Merged in: sage-4.6.2.alpha2
Authors: Andrey Novoseltsev Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

a sage (4.3.3) notebook shows the correct picture of

plot(x^2-5,(x,0,5),ymin=0)

The save method ignores the ymin parameter:

plot(x^2-5,(x,0,5),ymin=0).save("/tmp/test.png")

Dependency: #7981

Attachments (2)

trac_8632_save_ignores_options_from_plot.patch (1.1 KB) - added by novoselt 7 years ago.
trac_8632-reviewer.patch (763 bytes) - added by kcrisman 7 years ago.
reviewer patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by novoselt

See also #7981.

comment:2 Changed 7 years ago by novoselt

  • Milestone set to sage-4.6.2
  • Status changed from new to needs_review

With the current patch at #7981 this problem is gone. The graph goes a bit below ymin=0, but it happens in both cases in the same way, so save does not ignore the parameter anymore.

comment:3 Changed 7 years ago by novoselt

  • Cc kcrisman added

Easy review with #7981 applied ;-)

comment:4 Changed 7 years ago by kcrisman

  • Authors set to Andrey Novoseltsev
  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work
  • Work issues set to needs patch

Except we need a patch :-) It could go in the TESTS section; it would need to use the usual temp directory for Sage.

comment:5 follow-up: Changed 7 years ago by novoselt

Could you please remind me what is the usual temp directory for Sage?

Also, what exactly should the test do? How do I verify that images from show and save are the same? It seems like a waste of time on tests if it is only checked that these commands don't raise an exception - they were working before as well, just not as they should.

comment:6 in reply to: ↑ 5 Changed 7 years ago by kcrisman

Replying to novoselt:

Could you please remind me what is the usual temp directory for Sage?

See line 1732 of your patch for #7981 ;-) - kwds.setdefault('filename', sage.misc.misc.tmp_filename() + '.png')

Also, what exactly should the test do? How do I verify that images from show and save are the same? It seems like a waste of time on tests if it is only checked that these commands don't raise an exception - they were working before as well, just not as they should.

Sadly, we can't do that yet. (Matplotlib apparently does do this with Nose, but we don't have the capability yet.) Partly this could be useful for the future day when we CAN check things like this...

But for now the point is that at least someone can visually verify that there is a different min than $y=-5$ if they care to look. We want to document that we have done something about the particular one.

Alternately, you could try to ask a release manager to close this as a duplicate of #7981. Personally, I think it would be worth adding an example to the save documentation that one can choose to either put the commands in .save(foo=bar) or to pass it one from plot(f,foo=bar); that could be useful for a complete newbie to the code to see, so that they don't have to follow code around.

Changed 7 years ago by novoselt

comment:7 Changed 7 years ago by novoselt

  • Status changed from needs_work to needs_review

Thank you! How about this patch?

comment:8 Changed 7 years ago by kcrisman

  • Work issues needs patch deleted

I assume the patch depends on #7981, correct?

comment:9 Changed 7 years ago by novoselt

Yes!

comment:10 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

Positive review.

To buildbot: depends on #7981, apply trac_8632_save_ignores_options_from_plot.patch

comment:11 Changed 7 years ago by kcrisman

Scratch that. Need to change the patch a bit so documentation looks ok - a missing colon.

Changed 7 years ago by kcrisman

reviewer patch

comment:12 Changed 7 years ago by kcrisman

Okay, now all is well.

to buildbot: depends on #7981, apply trac_8632_save_ignores_options_from_plot.patch and trac_8632-reviewer.patch

comment:13 Changed 7 years ago by novoselt

Thanks for the corrections, sorry for sloppiness!

comment:14 follow-up: Changed 7 years ago by kcrisman

Just FYI - still applies fine on 4.6.2.alpha0.

comment:15 in reply to: ↑ 14 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

Replying to kcrisman:

Just FYI - still applies fine on 4.6.2.alpha0.

Actually, it doesn't:

patching file sage/plot/plot.py
Hunk #1 FAILED at 2394.
1 out of 1 hunk FAILED -- saving rejects to file sage/plot/plot.py.rej

comment:16 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to positive_review

Sorry, I missed the dependency on #7981.

comment:17 Changed 7 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 7 years ago by jdemeyer

  • Work issues rebase deleted
Note: See TracTickets for help on using tickets.