Opened 12 years ago

Last modified 8 years ago

#2100 closed enhancement

sensible defaults for aspect ratio — at Version 53

Reported by: jason Owned by: was
Priority: major Milestone: sage-4.7.2
Component: graphics Keywords: sd31
Cc: kcrisman, mhampton, mpatel Merged in:
Authors: Jason Grout, Karl-Dieter Crisman Reviewers: Andrey Novoseltsev, Karl-Dieter Crisman
Report Upstream: N/A Work issues: unify option names
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

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.

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

Change History (56)

comment:1 Changed 12 years ago by jason

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 11 years ago by mabshoff

Jason,

didn't we fix this recently?

Cheers,

Michael

comment:3 Changed 11 years ago by AlexGhitza

  • Type changed from defect to enhancement

comment:4 Changed 9 years ago by jason

  • Report Upstream set to 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 9 years ago by jason

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 9 years ago by jason

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 9 years ago by jason

  • Cc kcrisman added

comment:8 Changed 9 years ago by jason

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

comment:9 Changed 9 years ago by jason

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 9 years ago by jason

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

comment:11 Changed 9 years ago by jason

  • 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 9 years ago by jason

  • Description modified (diff)

Update the proposal.

comment:13 Changed 9 years ago by jason

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 follow-up: Changed 9 years ago by jason

  • Authors set to Jason Grout
  • Status changed from new to needs_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 9 years ago by jason

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

comment:16 in reply to: ↑ 14 Changed 9 years ago by jason

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 9 years ago by jason

apply instead of previous patch

comment:17 Changed 9 years ago by jason

  • Status changed from needs_work to needs_review

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

comment:18 Changed 9 years ago by jason

  • Description modified (diff)

Updating description to reflect changes in the patch.

comment:19 follow-up: Changed 9 years ago by novoselt

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 9 years ago by novoselt

  • Status changed from needs_review to needs_work
  • Work issues set to 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 9 years ago by novoselt

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

comment:22 Changed 9 years ago by jason

  • 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 9 years ago by kcrisman

  • Reviewers set to 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 9 years ago by kcrisman

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 9 years ago by jason

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 follow-up: Changed 9 years ago by 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.

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 9 years ago by kcrisman

  • Description modified (diff)

comment:28 in reply to: ↑ 26 ; follow-up: Changed 9 years ago by kcrisman

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 9 years ago by kcrisman

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 9 years ago by kcrisman

Rebase to 4.6.1/4.6.2.alpha0

comment:30 follow-up: Changed 9 years ago by kcrisman

  • Authors changed from Jason Grout to Jason Grout, Karl-Dieter Crisman
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Karl-Dieter Crisman
  • Status changed from needs_work to needs_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 9 years ago by kcrisman

  • Work issues complete documentation deleted

comment:32 in reply to: ↑ 30 Changed 9 years ago by kcrisman

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 9 years ago by novoselt

Depends on #10143

(others are "inherited")

comment:34 follow-up: Changed 9 years ago by novoselt

  • Status changed from needs_review to positive_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 9 years ago by kcrisman

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 follow-up: Changed 9 years ago by novoselt

  • Status changed from positive_review to needs_work
  • Work issues set to 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 9 years ago by novoselt

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 ; follow-up: Changed 9 years ago by 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.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 9 years ago by kcrisman

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 9 years ago by kcrisman

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 follow-up: Changed 9 years ago by novoselt

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 9 years ago by kcrisman

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 follow-up: Changed 9 years ago by novoselt

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 ; follow-up: Changed 9 years ago by kcrisman

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 9 years ago by kcrisman

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 9 years ago by kcrisman

  • Status changed from needs_work to needs_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 9 years ago by kcrisman

reviewer patch

comment:47 Changed 9 years ago by kcrisman

  • Work issues Plotting of vectors examples deleted

comment:48 follow-up: Changed 9 years ago by 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. 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 9 years ago by kcrisman

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 9 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to 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 9 years ago by kcrisman

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 9 years ago by kcrisman

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 9 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to needs_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!

Note: See TracTickets for help on using tickets.