Opened 15 years ago

Closed 11 years ago

#2100 closed enhancement (fixed)

sensible defaults for aspect ratio

Reported by: Jason Grout Owned by: Jason Grout
Priority: major Milestone: sage-4.7.2
Component: graphics Keywords: sd31
Cc: Karl-Dieter Crisman, mhampton, Mitesh Patel Merged in: sage-4.7.2.alpha0
Authors: Jason Grout, Karl-Dieter Crisman Reviewers: Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Karl-Dieter Crisman)

We have two concepts of "aspect ratio" that we'd like to expose to the user.

This patch exposes the following behavior:

    * Figure aspect ratio -- controls the final image size, including any text labels, etc. This will be set using the following options:
          o figsize
                + single number - use this as a base size for the width of the figure canvas
                + two numbers - an actual figure size in inches
                + None - use the default size in Matplotlib
          o fit_tight
                + if True, clip or extend the resulting figure to the plot objects (so the resulting figure will probably not have figsize dimensions
                + if False, the resulting figure has exactly figsize dimensions, but items in the plot may be clipped or there may be excessive padding around drawn objects
    * Pixel aspect ratio
          o aspect_ratio -- the ratio height/width for a unit square
                + 1 -- a unit square appears to have equal height and width.  This will be the default for Graphics objects (so if a default is not explicitly set, the default is this.)
                + 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure (default for plot and list_plot)
                + number -- ratio of height to width.

To patchbot/reviewers: Apply attachment:trac_2100-aspect-ratio-rebase.patch and attachment:trac_2100-auto-automatic-and-vector.patch

Attachments (6)

2100-aspect_ratio.2.patch (35.7 KB) - added by Jason Grout 12 years ago.
apply instead of previous patch
trac_2100-aspect-ratio-rebase.patch (35.8 KB) - added by Karl-Dieter Crisman 12 years ago.
Rebase to 4.6.1/4.6.2.alpha0
trac_2100-3d-vector.patch (2.7 KB) - added by Karl-Dieter Crisman 12 years ago.
reviewer patch
Before.png (22.1 KB) - added by Karl-Dieter Crisman 12 years ago.
What this plot looks like before #2100
After.png (14.8 KB) - added by Karl-Dieter Crisman 12 years ago.
What this plot looks like after #2100
trac_2100-auto-automatic-and-vector.patch (10.1 KB) - added by Karl-Dieter Crisman 11 years ago.
Apply after rebase patch

Download all attachments as: .zip

Change History (86)

comment:1 Changed 15 years ago by Jason Grout

For reference, in Mathematica (see http://reference.wolfram.com/mathematica/ref/AspectRatio.html?q=AspectRatio&lang=en )

  • Graphics objects use Automatic aspect ratio
  • plot and listplot use 1/golden ratio
  • parametric plot uses automatic
  • contour plot uses aspect ratio of 1

Automatic is an aspect ratio determined by the pixels in the plot. It does not distort the plot (i.e., circles are still circles).

comment:2 Changed 14 years ago by Michael Abshoff

Jason,

didn't we fix this recently?

Cheers,

Michael

comment:3 Changed 14 years ago by Alex Ghitza

Type: defectenhancement

comment:4 Changed 12 years ago by Jason Grout

Report Upstream: N/A

#9813 takes care of two cases of these.

Interestingly, from the above list, it seems like the golden ratio is only used for a minority of functions (i.e., "plot" and "list_plot"). So maybe we ought to change the global default to aspect_ratio=1 and just set plot and list_plot to have default golden ratio aspect ratios.

comment:5 Changed 12 years ago by Jason Grout

Matplotlib aspect ratio setting Matplotlib adjustable setting Mathematica AspectRatio setting Explanation
'auto' doesn't seem to matter Full Make the current data limits fill the available sized box
'equal' or 1 'box' Automatic Make each pixel be square (aspect ratio 1)

It appears that specifying a number in matplotlib ('box' adjustable) makes the *pixels* have a certain height-to-width ratio (e.g., a circle in the picture will appear to have the given aspect ratio), but in Mathematica, a number specifies the overall ratio of the *image* height-to-width.  Thus the numbers are not directly comparable.

Plus, in matplotlib, you can have the 'datalim' be adjustable, which changes the data limits on your plot to make the pixels have a certain aspect ratio.

comment:6 Changed 12 years ago by Jason Grout

Matplotlib's fig_aspect seems to do the sort of thing that mma's aspect ratio number setting does:

http://matplotlib.sourceforge.net/api/figure_api.html#matplotlib.figure.figaspect

comment:7 Changed 12 years ago by Jason Grout

Cc: Karl-Dieter Crisman added

comment:8 Changed 12 years ago by Jason Grout

One other note: setting bbox_inches='tight' in the savefig method seems to override the figure size specified.

comment:9 Changed 12 years ago by Jason Grout

We have two concepts of "aspect ratio" that we'd like to expose to the user.

  • Figure aspect ratio -- controls the final image size, including any text labels, etc.  This will be set using the following options:
    • fig_aspect_ratio
      • number - make an image with the specified aspect ratio (within reason) using the figaspect matplotlib function (which tries to be marginally smart, which actually may be a dumb thing to do...)
      • 'auto' (default) - use bbox_inches='tight' to create a figure that holds the drawn objects
    • figsize
      • single number - use this as a base size for fig_aspect_ratio calculations (i.e., bigger numbers = bigger figures)
      • two numbers - an actual figure size in inches
  • Pixel aspect ratio
    • aspect_ratio -- the aspect ratio of a pixel in the image (or of a unit square in data coordinates, for example).  The default here will change depending on the type of plot
      • 'equal' or 1 -- square pixels.  This will be the default for most things
      • 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure
      • number -- ratio of width to height (or height to width; I can't remember).  For plot() and list_plot(), this will default to giving a golden ratio aspect ratio
    • aspect_ratio_adjust -- what should we adjust to achieve the desired aspect ratio for the items drawn?  Note that by default, the axes limits are enlarged slightly; to eliminate this, set axes_pad=0
      • 'box' (default) -- the frame axes
      • 'datalim' -- the data limits

What do people think?

comment:10 Changed 12 years ago by Jason Grout

Helpful matplotlib mailing list post (read the thread): http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg15623.html

comment:11 Changed 12 years ago by Jason Grout

Description: modified (diff)

Moving the proposal up to the description so we can edit it. Ignore the proposal in the comments now.

comment:12 Changed 12 years ago by Jason Grout

Description: modified (diff)

Update the proposal.

comment:13 Changed 12 years ago by Jason Grout

I've attached two files.  First, a work-in-progress rewrite of the aspect ratio stuff.  Second, the plot.py file before this patch was applied.  The plot.py file is the version from 4.6.alpha1 with tickets #9740, #9746, and #4342 applied, in order.

comment:14 Changed 12 years ago by Jason Grout

Authors: Jason Grout
Status: newneeds_work

This ticket is still needs_work.  Remaining items include:

  • Updating docs to reflect changes, including
    • Most aspect ratios now default to 1
    • new/changed options:
      •  fig_tight
      • the new definition of aspect_ratio (= 1/old definition)
      • aspect_ratio_adjustable
  • Make GraphicsArray work (likely using the nice features of matplotlib in 1.0 for doing multiple subplots!)

Warning: this patch changes the definition of aspect_ratio.  Before, it was width/height, but now it is height/width.  The new meaning is consistent with matplotlib and Mathematica.

Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.

comment:15 Changed 12 years ago by Jason Grout

Also todo: set barchart and scatterplot to have default aspect ratios of 'auto'.

comment:16 in reply to:  14 Changed 12 years ago by Jason Grout

Replying to jason:

Warning: this patch changes the definition of aspect_ratio. Before, it was width/height, but now it is height/width. The new meaning is consistent with matplotlib and Mathematica. Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.

Apparently this statement isn't true. The old aspect_ratio and new aspect_ratio mean the same thing. I was basing the statement on reading the docs. Either the current docs are backwards, or I was misreading them.

So, in short, things are good to go for finishing this patch and getting it into Sage as soon as possible.

Changed 12 years ago by Jason Grout

Attachment: 2100-aspect_ratio.2.patch added

apply instead of previous patch

comment:17 Changed 12 years ago by Jason Grout

Status: needs_workneeds_review

New patch revamping the aspect ratio calculations in Sage (and defaults!)

comment:18 Changed 12 years ago by Jason Grout

Description: modified (diff)

Updating description to reflect changes in the patch.

comment:19 Changed 12 years ago by Andrey Novoseltsev

It seems to me that set_aspect ratio only accepts "auto" as a string input, while the proposal also lists "equal" as an option.

Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for width=\textwidth or height=0.5\texthight and have the second dimension determined automatically based on the actual plot and aspect ratio.

comment:20 Changed 12 years ago by Andrey Novoseltsev

Status: needs_reviewneeds_work
Work issues: complete documentation

New functions pad_for_tick_labels and adjust_figure_to_contain_bbox are not completely documented.

Are #9740, #9746, and #4342 still prerequisites?

comment:21 Changed 12 years ago by Andrey Novoseltsev

Oops, got the answer to the last question already, for some reason they were not crossed in the above comment.

comment:22 Changed 12 years ago by Jason Grout

Cc: mhampton added

This might be good bug-day fodder if people are already working on graphics code.

comment:23 in reply to:  19 Changed 12 years ago by Karl-Dieter Crisman

Reviewers: Andrey Novoseltsev

I hope to take care of novoselt's comments - 'equal' and the documentation - today. This will depend on #7981, #8632, #10244, and #10143.

Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for width=\textwidth or height=0.5\texthight and have the second dimension determined automatically based on the actual plot and aspect ratio.

I think this is a good idea, but I don't think the people who might work on this will have time for it before bitrot sets in even further. I have opened ticket #10633 for this.

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

Rather impressively, only one of seven hunks failed in contour_plot.py, and only 5 of 34 hunks failed on plot.py. So rebasing this to the other recent graphics patches should be no problem, as well as the other documentation comments.

comment:25 Changed 12 years ago by Jason Grout

Let's just take the 'equal' option out of the proposal. I think it's there just because that's what matplotlib does.

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

I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().

I can also remove 'equal' if you want, that's fine.

Before I go ahead and update this, though, I have a question. The two functions not documented (pad_for_tick_labels and adjust_figure_to_contain_bbox) only are used in a commented-out line

            # this messes up the aspect ratio!
            #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)

So... should they even be included at this time?

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

Description: modified (diff)

comment:28 in reply to:  26 ; Changed 12 years ago by Karl-Dieter Crisman

Replying to kcrisman:

I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().

I can also remove 'equal' if you want, that's fine.

Done as well.

Before I go ahead and update this, though, I have a question. The two functions not documented (pad_for_tick_labels and adjust_figure_to_contain_bbox) only are used in a commented-out line

            # this messes up the aspect ratio!
            #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)

So... should they even be included at this time?

Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."

New patch coming up relatively soon.

comment:29 in reply to:  28 Changed 12 years ago by Karl-Dieter Crisman

So... should they even be included at this time?

Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."

Since I didn't really know where these should be used, or what the context was, I decided to just leave them in but fully commented out, with an admonition to add documentation if they ever get used again. (This would also make it easier for a future developer to use them, inter alia.)

Getting some doctest failures, though, related to this:

sage -t  devel/sage/sage/plot/plot3d/plot_field3d.py # 6 doctests failed

I doubt this will be hard to resolve. Anyway, just status updates...

Changed 12 years ago by Karl-Dieter Crisman

Rebase to 4.6.1/4.6.2.alpha0

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

Authors: Jason GroutJason Grout, Karl-Dieter Crisman
Reviewers: Andrey NovoseltsevAndrey Novoseltsev, Karl-Dieter Crisman
Status: needs_workneeds_review

The problem with the plot_field3d turns out to be the following very bad behavior of plotting vectors after this patch

sage: v = vector([1.,2.,3.])
sage: v.plot() # all is well
sage: plot(v) # passes in aspect_ratio to a 3d graphics, which already has such a thing, and get nasty traceback

The culprit was telling plot_vector_field3d to do plot(v) for each vector instead of v.plot() as it should have. However, this exposes something else really dumb - namely, that plot.py turns vectors into tuples before it plots them

    from sage.structure.element import is_Vector
    if kwds.get('parametric',False) and is_Vector(funcs):
        funcs = tuple(funcs)


    if hasattr(funcs, 'plot'):
        G = funcs.plot(*args, **original_opts)
    # if we are using the generic plotting method
    else:

this presumably avoids the fact that

sage: w = vector([x^2,3,x^3])
sage: plot(w,(x,0,1))
---------------------------------------------------------------------------
NotImplementedError                   

but still this is a problem. So after fixing this issue, I'm opening a ticket for making vector plotting more sensible.

I think this change might need review (?) so I'm setting to 'needs review'.

To buildbot and reviewer: apply trac_2100-aspect-ratio-rebase.patch and trac_2100-3d-vector.patch. Depends on #7981, #8632, #10244, and #10143.

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

Work issues: complete documentation

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

I think this change might need review (?) so I'm setting to 'needs review'.

Oh yeah, and I never actually looked at very many of the plots. Definitely still needs review ;) of the pictures and the (small) reviewer patch.

comment:33 Changed 12 years ago by Andrey Novoseltsev

Depends on #10143

(others are "inherited")

comment:34 Changed 12 years ago by Andrey Novoseltsev

Status: needs_reviewpositive_review

I'm setting this to positive review - it is a great improvement and it is a shame that it took us so long ;-)

I am fine with the reviewer patch and I did take a look at a few (although definitely not all) plots in the documentation. Circles are finally circles!!!

I have also discovered the following behaviour which may be a bug:

sage: x,y = var('x,y') 
sage: print x, y
sage: f(x,y) = x^2 + y^2 - 2 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)

The first line (with fill=True) completely fills the plot, not just the disk.

comment:35 in reply to:  34 Changed 12 years ago by Karl-Dieter Crisman

Thanks!

I have also discovered the following behaviour which may be a bug:

sage: x,y = var('x,y') 
sage: print x, y
sage: f(x,y) = x^2 + y^2 - 2 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)

The first line (with fill=True) completely fills the plot, not just the disk.

This also happens with e.g. 4.6.1.alpha3, so this is definitely not related. It sounds a LOT like #9744. It'd be great if you could confirm that. I thought we'd already taken care of this... unfortunately not, it seems.

comment:36 Changed 12 years ago by Andrey Novoseltsev

Status: positive_reviewneeds_work
Work issues: Plotting of vectors examples

Actually, I was too hasty:

sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1390:
    sage: plot(v) # defaults to an arrow plot
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[9]>", line 1, in <module>
        plot(v) # defaults to an arrow plot###line 1390:
    sage: plot(v) # defaults to an arrow plot
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1391:
    sage: plot(v, plot_type='arrow')
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[10]>", line 1, in <module>
        plot(v, plot_type='arrow')###line 1391:
    sage: plot(v, plot_type='arrow')
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1393:
    sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list())
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[12]>", line 1, in <module>
        plot(v, plot_type='point')+frame3d((Integer(0),Integer(0),Integer(0)), v.list())###line 1393:
    sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list())
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
1 items had failures:
   3 of  17 in __main__.example_45
***Test Failed*** 3 failures.

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

comment:37 Changed 12 years ago by Andrey Novoseltsev

Yes, the problem I hit is exactly #9744 (which actually mentions the same example from the documentation), thanks for pointing it out.

comment:38 in reply to:  36 ; Changed 12 years ago by Karl-Dieter Crisman

Replying to novoselt:

Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"

Aargh! I'll try to fix this...

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

comment:39 in reply to:  38 ; Changed 12 years ago by Karl-Dieter Crisman

Replying to kcrisman:

Replying to novoselt:

Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"

Aargh! I'll try to fix this...

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

The correct fix is to remove that kwd from the plot when

        if plot_type == 'arrow' or plot_type == 'point':
            dimension = len(coords)
            if dimension == 3:
                from sage.plot.plot3d.shapes2 import line3d, point3d
                
                if plot_type == 'arrow':
                    return line3d([(0,0,0), coords], arrow_head=True, **kwds)
                else:
                    return point3d(coords, **kwds)

that should do it. I will try this later.

comment:40 in reply to:  39 Changed 12 years ago by Karl-Dieter Crisman

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

Okay, now I am not rushing out the door and can think straight. The problem is that this was never doctesting the thing in question in the first place because of the issues raised in #10638.

I think the issue is that _extract_kwds_for_show keeps the @options value of aspect_ratio='auto') in play; when this gets passed to the show() method of Line (which is what line3d creates in this case), it refers to the one in base.PrimitiveObject?, which unfortunately then puts this value of aspect_ratio` in:

        opts = {}
        opts.update(SHOW_DEFAULTS)
        if self._extra_kwds is not None:
            opts.update(self._extra_kwds)
        opts.update(kwds)

and then doesn't realize it's a problem in

        if opts['frame_aspect_ratio'] == 'automatic':
            if opts['aspect_ratio'] != 'automatic':
                # Set the aspect_ratio of the frame to be the same as that of
                # the object we are rendering given the aspect_ratio we'll use
                # for it.
                opts['frame_aspect_ratio'] = \
                    self._determine_frame_aspect_ratio(opts['aspect_ratio'])

Which is where the problem happens.

Although fixing this in the vector plotting didn't seem to work immediately.

Unless we wanted to change all the 'auto' in this patch to 'automatic'. I don't know if that is desirable either, though. What do others think?

comment:41 Changed 12 years ago by Andrey Novoseltsev

I like auto more than automatic, so I don't want to change it.

Can we maybe change it the other way? Was automatic documented/used somewhere before?

comment:42 in reply to:  41 Changed 12 years ago by Karl-Dieter Crisman

I like auto more than automatic, so I don't want to change it.

Agreed, but...

Can we maybe change it the other way? Was automatic documented/used somewhere before?

Yes, this is the standard 3D option. See this reference manual page for plot/plot3d/base.pyx.

Of course, it would be possible to change the plot3d case to also accept 'auto' easily enough - in fact, that's essentially what I've done in my latest patch - and one could change the documentation accordingly. But we couldn't remove it without a deprecation period - and in my mind it's pointless to go to the trouble, given that it's mostly a default so we could change things without even needing to deprecate that option.

comment:43 Changed 12 years ago by Andrey Novoseltsev

I guess then changing to automatic is OK.

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

comment:44 in reply to:  43 ; Changed 12 years ago by Karl-Dieter Crisman

Replying to novoselt:

I guess then changing to automatic is OK.

I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

comment:45 in reply to:  44 Changed 12 years ago by Karl-Dieter Crisman

I guess then changing to automatic is OK.

I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).

One reason being that matplotlib uses 'auto', and consistency with that is nice as well.

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

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

Status: needs_workneeds_review

Okay, this latest reviewer patch should fix the things we've talked about in a minimalist way. It doesn't resolve the 'auto' versus 'automatic' issue. No new instructions for applying patches. Documentation also looks good with this change.

Changed 12 years ago by Karl-Dieter Crisman

Attachment: trac_2100-3d-vector.patch added

reviewer patch

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

Work issues: Plotting of vectors examples

comment:48 Changed 12 years ago by Jason Grout

Wow, great job tracking down this subtle problem. I personally slightly prefer 'auto' over 'automatic' because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so 'automatic' would probably be fine as well. If we do make the default 'automatic', then I think we'll have to convert it to 'auto' before passing it to matplotlib.

Another data point: mma uses Automatic (not Auto): http://reference.wolfram.com/mathematica/ref/Automatic.html

comment:49 in reply to:  48 Changed 12 years ago by Karl-Dieter Crisman

Replying to jason:

Wow, great job tracking down this subtle problem. I personally slightly prefer 'auto' over 'automatic' because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so 'automatic' would probably be fine as well.

Thanks. I don't have any more time today to work on this, but I think then I vote for 'automatic' for consistency, and having 'auto' as one that secretly also works :)

If we do make the default 'automatic', then I think we'll have to convert it to 'auto' before passing it to matplotlib.

Yes, I realize that. I don't think that will be very difficult, since there seems to be only one place this is passed.

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

Status: needs_reviewneeds_work
Work issues: unify option names

Okay, putting this to 'needs work' in order to fix this. Hopefully today or this weekend; it won't be hard, just have to do it.

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

Oops... hmm, I gave myself one long weekend. I'll try to see if this still applies to 4.6.2.alpha4 soon.

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

All still applies to 4.6.2.alpha4. I hope to conclude this adventure shortly, by replacing the reviewer patch with one that deals with the automatic/auto issue uniformly.

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

Description: modified (diff)
Status: needs_workneeds_review

This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size. The new reviewer patch is now different enough that it needs review. Please do so now to avoid bitrot!

comment:54 in reply to:  53 Changed 12 years ago by Jason Grout

Replying to kcrisman:

This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size.

Yes, I reverted things to the matplotlib defaults. Before, we made the ratio of height/width the golden ratio, but now it is the standard matplotlib default (which I believe is 4:3, which matches computer screens better anyway).

comment:55 Changed 12 years ago by Jason Grout

Owner: changed from William Stein to Jason Grout

Can you add instructions for reviewers/patchbot about which patches should be applied in what order?

comment:56 Changed 12 years ago by Jason Grout

(and then I'll try to review this tonight or tomorrow)

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

Description: modified (diff)

See the description. I guess I didn't make it obvious enough :)

Apply [trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch] for this.

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

Description: modified (diff)

Apply [trac_2100-aspect-ratio-rebase.patch trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch trac_2100-auto-automatic-and-vector.patch] for this.

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

I can't get the links to work for some reason - sorry.

comment:60 in reply to:  58 Changed 12 years ago by Karl-Dieter Crisman

Description: modified (diff)

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

Here is some code that looks less good with this patch than before.

def my_eulers_method_plot(a_function,x0,y0,h,x1):
    n=int((1.0)*(x1-x0)/h)
    x00=x0; y00=y0
    x01=x0; y01=y0
    P=point((x00,y00),rgbcolor=hue(1)) # red    
    Q=Graphics() # default is blue
    f(x,y)=a_function(x,y) # Note that a_function should be a callable function of x, then y
    for i in range(n+1):
        y01 = y00+h*f(x00,y00)
        x01 = x00+h
        P=P+point((x01,y01),rgbcolor=hue(1))
        Q=Q+line([(x00,y00),(x01,y01)])
        x00=x01
        y00=y01
    return(P+Q)

var('x,y')
def euler_logistic_plot(parameter,y_start,end=15,step=1):
    function(x,y)=parameter*y*(1-y)
    my_eulers_method_plot(function,0,y_start,step,end).show(ymin=0)

euler_logistic_plot(2,.7,97,.8)

I'll try to attach before and after.

Changed 12 years ago by Karl-Dieter Crisman

Attachment: Before.png added

What this plot looks like before #2100

Changed 12 years ago by Karl-Dieter Crisman

Attachment: After.png added

What this plot looks like after #2100

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

Status: needs_reviewneeds_info

The issue is of course that I never used plot() to do this, but rather only line() and point() and Graphics(). The default aspect ratio seems to have changed a lot, and so not only are the labels on the left totally squished, but it just looks bad. Maybe mpl defaults aren't that great? Or is this a pretty unusual scenario?

Anyway, putting as needs info until I know what is happening here from someone who knows more about mpl (e.g., Jason). So frustrating, too, because we should have this in!

comment:63 Changed 12 years ago by Jason Grout

Yep, I think the problem here is that line and point now default to have aspect_ratio=1, so the y and x axis are drawn at equal scales. Try setting the aspect_ratio to 'auto' (or whatever your patch does now).

plot() defaults to 'auto' aspect ratio (e.g., fill the figure). However, geometric figures like line and point default to aspect_ratio=1.

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

I think perhaps we should make line (and point?) default to 'auto'? Because one is quite likely to have a line do a function, as opposed to other geometric figures. And points are often used to shadow functions.

Any other thoughts on this? It would be really nice to finally get this in 4.7 - maybe you can take a quick look over spring break?

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

Status: needs_infoneeds_review
Work issues: unify option names

Putting needs review, though may need a minor change because of the thing in the last two comments.

comment:66 Changed 11 years ago by Karl-Dieter Crisman

Keywords: sd31 added

Okay, I'm going to refresh so that line and point still have 'auto'. Needs review!

comment:67 Changed 11 years ago by Ryan

Reviewers: Andrey Novoseltsev, Karl-Dieter CrismanAndrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout

FYI, I am currently reviewing this patch

comment:68 Changed 11 years ago by Ryan

Status: needs_reviewpositive_review

Patch looks good. Confirmed that kcrisman's code looks 'good' after the patch too :) Positive Review. Great job guys.

Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.

comment:69 in reply to:  68 Changed 11 years ago by Andrey Novoseltsev

Replying to ryan:

Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.

What exactly do you mean by this? That a clean public release of Sage has doctest failures in plot_field.py?

comment:70 Changed 11 years ago by Karl-Dieter Crisman

Correct - or to be more precise, warnings that do not cause the doctest to fail. They are known, and not from Sage.

comment:71 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.1sage-4.7.2

comment:72 Changed 11 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work
Work issues: rebase

Please rebase this patch to be applied on top of #11491.

comment:73 in reply to:  72 ; Changed 11 years ago by Karl-Dieter Crisman

Replying to jdemeyer:

Please rebase this patch to be applied on top of #11491.

Thanks - another rebase where a major improvement takes second rank to a trivial adjustment. Especially since Ryan and I were involved on both tickets, it would have been nice to ask first. Rebasing is not so trivial for the setups some of us have.

But no point crying over spilt milk. Sigh... patches coming up.

Changed 11 years ago by Karl-Dieter Crisman

Apply after rebase patch

comment:74 Changed 11 years ago by Karl-Dieter Crisman

Status: needs_workpositive_review

Okay, now we should hopefully be at positive review!

comment:75 in reply to:  73 Changed 11 years ago by Jeroen Demeyer

Replying to kcrisman:

Especially since Ryan and I were involved on both tickets, it would have been nice to ask first.

I have done that in the past and people complained: http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5

comment:76 Changed 11 years ago by Karl-Dieter Crisman

Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.

What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.

Anyway, it turned out I had time for fixing it - it was a very small overlap, luckily. Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.

comment:77 in reply to:  76 ; Changed 11 years ago by Jeroen Demeyer

Work issues: rebase

Replying to kcrisman:

Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.

What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.

It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.

Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.

comment:78 in reply to:  76 Changed 11 years ago by Jeroen Demeyer

Cc: Mitesh Patel added

Replying to kcrisman:

Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.

You should really also thank Mitesh Patel for managing the buildbot.

comment:79 in reply to:  77 Changed 11 years ago by Karl-Dieter Crisman

It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.

Understood.

Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.

Yes, that definitely makes sense, as I hope I make clear above. :)

comment:80 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.2.alpha0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.