Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Square plots #4310

Merged
merged 8 commits into from Jun 16, 2015
Merged

Square plots #4310

merged 8 commits into from Jun 16, 2015

Conversation

insertroar
Copy link
Contributor

Implemented feature suggested in #3742 .
Added square parameter to the axis method, which sets equal limits and equal scale on both axes.
Test case with image comparison included.

@insertroar insertroar changed the title Added square plot parameter to axes Square plots Apr 4, 2015
self.set_autoscale_on(False)
xlim = self.get_xlim()
ylim = self.get_ylim()
minlim = min(xlim[0], ylim[0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would in be better to change the limits to

edge_size = max(np.diff(xlim), np.diff(ylim))
ax.set_xlim([xliml[0], xlim[0] + edge_size])
ax.set_ylim([ylim[0], ylim[0] + edges_size])

Which would let you get a square area, but with the lower left any where is dataspace, not just on the x==y diagonal.

@tacaswell
Copy link
Member

This will also need to be documented in https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new

Looks like a good start!

@tacaswell tacaswell added this to the next point release milestone Apr 5, 2015
@tacaswell
Copy link
Member

This needs to be re-based.

@insertroar
Copy link
Contributor Author

Done, re-based, is there anything else to add to it?

Square plot:

Added implementation of square plot feature that sets the axes of a plot
to have the same maximum difference between them, and set the scale to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very awkwardly worded, maybe say xmax-xmin == ymax-ymin?

@tacaswell
Copy link
Member

@efiring thoughts?

@efiring
Copy link
Member

efiring commented Apr 13, 2015

I also thought about zoom/pan; my conclusion is that enforcing 'square' when zooming and panning is much more trouble than it is worth. I don't see a use case for it. It seems to me that 'square' is something one specifies in a script for a final plot, not something that matters when zooming and panning. There, maintaining the aspect ratio does matter, and this is already handled.

@tacaswell
Copy link
Member

Sounds reasonable 👍 On this from me (modulo documentation issues).

and the aspect ratio of the plot. For details, see
:func:`~matplotlib.pyplot.axis`.
Call signature::

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you are re-writing this, can you convert it to numpydoc format as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I wasn't aware you're encouraging this format, and it's not clear from the current docstrings. Before I commit, is this ok? Seeing as the function only takes in optional arguments, I wasn't sure if to include a 'Parameters' section or to put both *v and **kwargs in 'Other parameters'

"""Set axis properties

        Convenience method for manipulating axis properties such as
        the x and y view limits and the aspect ratio of the plot.

        For details, see
        :func:`~matplotlib.pyplot.axis`.

        Parameters
        ----------
        *v
            Axis toggle, aspect ratio and limits properties.

        Other parameters
        ----------------
        **kwargs
            Axis limit properties

        *kwargs* are passed on to :meth:`set_xlim` and
        :meth:`set_ylim`

        Returns
        -------
        v : list of axis limits ``(xmin, xmax, ymin, ymax)``.

        """

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, I think you will need to escape those *s with backslashes, and also I don't think you need :meth: or :func:, though I may have got that wrong.

Before you commit you can check it works by switching to the matplotlib/doc folder and running make.py html which will generate the doc and place it in doc/build/html, note it will take a few minutes to do it the first time, following runs it will only build the files that have changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the docstrings should be numpydoc formatted, we just have a huge backlog to get through (and updating docstrings is even less sexy than triaging bugs).

Looking at this more cloesly this function does not fit well with numpydoc as it uses patterns that the numpy developers don't use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, please correct me if I am wrong, but it looks like there should never be more than one arg as the logic is

if len(v) == 1 and isinstance(str, v[0]):
     # parse string
elif not len(v):
    # look work for kwargs
else:
   v = v[0]
   assert len(v) == 4
   # do stuff with the vector

so I think a docstring like

If no positional or kwargs are given, returns the current axes limits are returned with no side effects.

If a positional argument is passed the keyword arguments are ignored.

If no positional argument is passed, the keyword arguments are used.

Parameters
----------
v : list of float or {'on', 'off', 'equal', 'tight', 'scaled', 'normal', 'auto', 'image', 'square'}
    Optional positional argument

    Value to set the axis settings to.  If a list it is interpreted as [xmin, xmax, ymin, ymax].
    If a string 

       INSERT TABLE HERE

emit : bool, optional
    Passed to  set_{x,y}lim functions, if observers are notified of axis limit change

xmin, ymin, xmax, ymax : float, optional
    The axis limits to be set

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er, forget

Returns
-------
xmin, xmax, ymin, ymax : float
    The axis limits

Parameters
----------
v : list of float or {'on', 'off', 'equal', 'tight', 'scaled',\
'normal', 'auto', 'image', 'square'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you build the docs does this look ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the \ puts everything on one line. The only thing is the table size:
axis_doc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table looks fine to me size wise. There are a lot of options so it
needs lot of descriptions.

On Sun, Apr 19, 2015 at 5:39 PM basharovV notifications@github.com wrote:

In lib/matplotlib/axes/_base.py
#4310 (comment):

  •    Convenience method for manipulating the x and y view limits
    
  •    and the aspect ratio of the plot. For details, see
    
  •    :func:`~matplotlib.pyplot.
    

axis`.

  •    """Set axis properties.
    
  •    If no positional or kwargs are given, returns the current
    
  •    axes limits are returned with no side effects.
    
  •    If a positional argument is passed the keyword arguments are ignored.
    
  •    If no positional argument is passed, the keyword arguments are used.
    
  •    Parameters
    

  •    v : list of float or {'on', 'off', 'equal', 'tight', 'scaled',\
    
  •     'normal', 'auto', 'image', 'square'}
    

Yes, the \ puts everything on one line. The only thing is the table size:
[image: axis_doc]
https://cloud.githubusercontent.com/assets/8410950/7221487/d00fd61c-e6ba-11e4-91d2-dbca6173cf28.png


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4310/files#r28657228.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was referring to the actual padding in the table. It's not consistent with the other tables, but that's just an aesthetic thing.

@efiring
Copy link
Member

efiring commented Apr 20, 2015

I don't want to bog you down in documentation fixups that are peripheral to your PR; feel free to ignore my comments.

@tacaswell
Copy link
Member

Particularity when the text is very close to what I wrote...

On Sun, Apr 19, 2015 at 11:25 PM Eric Firing notifications@github.com
wrote:

I don't want to bog you down in documentation fixups that are peripheral
to your PR; feel free to ignore my comments.


Reply to this email directly or view it on GitHub
#4310 (comment)
.

@basharovV
Copy link
Contributor

Thanks for the input! Regarding the PR - yes this is a bit off-track but that doesn't mean I should ignore your comments.

changes the limit ranges (*xmax*-*xmin*) and (*ymax*-*ymin*) of
the *x* and *y* axes to be the same, and have the same scaling,
resulting in a square plot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't axis()'s docstring in pyplot autogenerated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's outside the autogenerated part, running boilerplate.py doesn't affect it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are two for two in confusing devs on this!

Which does raise the question why isn't this part of boilerplate generated
code?

On Mon, Apr 20, 2015, 18:40 basharovV notifications@github.com wrote:

In lib/matplotlib/pyplot.py
#4310 (comment):

@@ -1406,6 +1406,12 @@ def axis(_v, *_kwargs):
as kwargs selectively to alter just those limits without changing
the others.

  •  >>> axis('square')
    
  • changes the limit ranges (xmax-xmin) and (ymax-ymin) of
  • the x and y axes to be the same, and have the same scaling,
  • resulting in a square plot.

It's outside the autogenerated part, running boilerplate.py doesn't affect
it.


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4310/files#r28736156.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boilerplate generation is applied only to plotting functions. It adds thegca(), the hold kwarg, and the draw_if_interactive. There are many functions that are only in pyplot. If I remember correctly, axis started out as pyplot-only but was added to Axes later. Certainly boilerplate.py could be modified to handle more Axes methods, but I don't think there is any point in doing anything like that until various other major changes settle down.

@efiring
Copy link
Member

efiring commented May 14, 2015

Is this waiting for anything more to be done? It looks to me like it is ready to merge.

@WeatherGod
Copy link
Member

I thought what remained was to synchronize the pyplot docstring and the axes docstring

@efiring
Copy link
Member

efiring commented May 14, 2015

I think that the way this PR handles the pyplot docstring is fine. It modifies it in accord with its present style to take into account the new kwarg. This looks to me like it will be much more helpful to the user, especially the new user, than simply duplicating the opaque numpydoc docstring from Axes.

tacaswell added a commit that referenced this pull request Jun 16, 2015
ENH: 'square' argument to `Axes.axis`
@tacaswell tacaswell merged commit 42a852d into matplotlib:master Jun 16, 2015
@tacaswell
Copy link
Member

@insertroar Thank you

@tacaswell tacaswell mentioned this pull request Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants