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

Wrap legend #333

Merged
merged 28 commits into from
Oct 31, 2019
Merged

Wrap legend #333

merged 28 commits into from
Oct 31, 2019

Conversation

liamtoney
Copy link
Member

@liamtoney liamtoney commented Oct 8, 2019

Description of proposed changes

A wrapper for the GMT 6 legend command with support for specifying "handles" and "labels" as arguments, as in MATLAB (legend docs) and matplotlib (legend docs). The plotting commands (i.e., gmt.Figure.plot(), for now) are modified to return "handles" which are provided as inputs to gmt.Figure.legend(). This addresses part of #214 and #231.

Fixes #260

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

pygmt/clib/session.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added this to the 0.1.0 milestone Oct 8, 2019
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Hi @liamtoney, just checking in on how you're doing. If you're stuck with finding a specfile, maybe try to pull in the @Table_5_11.txt dataset. You can look at GenericMappingTools/gmt#1749 (comment) for some inspiration (though the auto-legend won't work yet).

I also see that pylint is complaining a lot so I've suggested a few changes. Run make lint to see what the other problems are, and try your best to resolve those. If it's too hard, we can try to resolve them for you or teach you how to set an ignore flag (if it proves to be impossible).

pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Oct 12, 2019

The auto-legend feature has been merged into GMT 6.0 branch in GenericMappingTools/gmt#1749. Now we can use -l option in the plot module, which enables legends automatically. Below is a simple script showing how we can get auto-legends even without calling the legend module explicitly.

gmt begin plot png
  gmt plot -R0/7/3/7 -Jx1i -B @Table_5_11.txt -Sc0.15i -Glightgreen -Wfaint -lApples
  gmt plot @Table_5_11.txt -W1.5p,gray -l"My Lines"
  gmt plot @Table_5_11.txt -St0.15i -Gorange -lOranges
gmt end show

This is what we will get:
image

With the auto-legend feature merged, it's almost straightforward to wrap the legend module in pygmt, and we can write a Python script like:

import pygmt
fig = pygmt.Figure()
fig.basemap(projection="x1i", region="0/7/3/7", frame=True)
fig.plot("@Table_5_11.txt", style="c0.15i", color="lightgreen", pen="faint", label="Apples")
fig.plot("@Table_5_11.txt", pen="1.5p,gray", label="My lines")
fig.plot("@Table_5_11.txt", style="t0.15i", color="orange", label="Oranges")
fig.show()

The only problem is, GMT 6.0.0 is not official released yet. It shouldn't take too long. We plan to release GMT 6.0.0 when the GMT6 paper is officially published.

@liamtoney
Copy link
Member Author

Great. This should simplify the wrapper a lot. From a dev perspective, to build and test this wrapper do I now need to clone the GMT repo and build from source? Or are there plans for another conda release?

@weiji14
Copy link
Member

weiji14 commented Oct 16, 2019

We're still using 6.0.0rc4 for pygmt development, and that's what our code is tested against. I think we should wrap this up and refactor the code after 6.0.0 stable is released. The implementation doesn't have to be perfect, having something is better than nothing!

@seisman
Copy link
Member

seisman commented Oct 16, 2019

FYI, the GMT team is planing to release 6.0.0rc5 (with the auto-legend feature included) next week, and release the final 6.0.0 on Nov. 1.

@weiji14
Copy link
Member

weiji14 commented Oct 24, 2019

Right, it's not quite as easy as it seems... The auto-legend only gets triggered when we run gmt end, so the following won't work:

import pygmt
fig = pygmt.Figure()
fig.basemap(projection="x1i", region="0/7/3/7", frame=True)
fig.plot(data="@Table_5_11.txt", style="c0.15i", color="lightgreen", pen="faint", l="Apples")
fig.plot(data="@Table_5_11.txt", pen="1.5p,gray", l="My_lines")
fig.plot(data="@Table_5_11.txt", style="t0.15i", color="orange", l="Oranges")
fig.show()

Produces:

Figure no legend

I'm assuming though that calling legend would trigger the legend being plotted, is that correct @seisman? I can start on some side work to wrap a "label" alias for text (and other functions). Plus we need to find a way to handle blank spaces so that label="My lines" works instead of having label="My_lines".

@seisman
Copy link
Member

seisman commented Oct 24, 2019

I think you're right. With pygmt, we have to call legend to plot the hidden legend specfile.

@liamtoney
Copy link
Member Author

So could the code have an empty legend call after plotting, e.g.

import pygmt
fig = pygmt.Figure()
fig.basemap(projection="x1i", region="0/7/3/7", frame=True)
fig.plot(data="@Table_5_11.txt", style="c0.15i", color="lightgreen", pen="faint", l="Apples")
fig.plot(data="@Table_5_11.txt", pen="1.5p,gray", l="My_lines")
fig.plot(data="@Table_5_11.txt", style="t0.15i", color="orange", l="Oranges")
fig.legend()  # Empty legend call
fig.show()

where fig.legend() when called with no arguments somehow accesses the hidden specfile if available?

@weiji14
Copy link
Member

weiji14 commented Oct 24, 2019

Hmm, I just tried using an empty fig.legend() on your branch (after making sure spec=None is allowed in the base_plotting.py's legend function) and it's complaining that the -D (a.k.a position) argument is needed. So again, not very automatic but at least we can get rid of the regex bits 🤷‍♂️

@weiji14
Copy link
Member

weiji14 commented Oct 24, 2019

Ah wait, I just noticed a new -T argument mentioned in the modern mode auto-legend note that outputs a specfile. We could perhaps set that to True if no specfile is given (i.e just a blank legend call) and use the auto-generated specfile to auto-generate the legend, what do you think?

Edit: Argh! Even with -T, we still need to use -D/position! Ok, just ignore this idea.

Now it tests the `position` argument
@liamtoney
Copy link
Member Author

@weiji14, @seisman, @leouieda — what do you think about having a default argument for position? Say, JTR+jTR to by default place the legend in the top-right corner, inside. That way a user could have more fluid experience by simply calling pygmt.legend().

@weiji14
Copy link
Member

weiji14 commented Oct 29, 2019

Oops, sorry, hit the close button by accident...

I was thinking the same thing. The thing is, GMT does have a default position setting that gets triggered by calling end, and we would be 'reinventing the wheel' a bit here.

@weiji14 weiji14 reopened this Oct 29, 2019
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Right, just a couple of minor edits to the test file (you're almost there!). Plus you'll need to add a line to the docs/api/index.rst file so that your shiny legend shows up in the docs! Do it like so:

    Figure.grdimage
    Figure.legend
    Figure.logo

Once you're done, remove the "WIP" from the title and it should be good to go 🚀. Let's open up a separate pull request to get that default position feature into PyGMT since this one is already getting way too long.

pygmt/tests/test_legend.py Outdated Show resolved Hide resolved
pygmt/tests/test_legend.py Show resolved Hide resolved
pygmt/tests/test_legend.py Show resolved Hide resolved
@liamtoney liamtoney changed the title WIP Wrap legend Wrap legend Oct 31, 2019
@liamtoney
Copy link
Member Author

@weiji14 I've made those changes. My commit history is a little messy (sorry) — does anything need to be done about that prior to merging?

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Hi @liamtoney, just one last change! And no you don't have to worry about the commit history since we'll be squash merging (i.e. it'll only show up as a single commit on the master branch).

pygmt/tests/test_legend.py Outdated Show resolved Hide resolved
Co-Authored-By: Wei Ji <23487320+weiji14@users.noreply.github.com>
@weiji14 weiji14 merged commit 13dfb56 into GenericMappingTools:master Oct 31, 2019
@liamtoney liamtoney deleted the add-legend branch October 31, 2019 19:16
@liamtoney
Copy link
Member Author

Thanks for walking me through this, @weiji14!

@weiji14
Copy link
Member

weiji14 commented Oct 31, 2019

Phew, good work @liamtoney. Thanks for having the stamina to see this one through! Feel free to open up another pull request to get the auto-position done (and we can debate around the implementation 😆). Also it'd be great to have a gallery example for legend on the website.

@@ -181,6 +181,8 @@ def grdcontour(self, grid, **kwargs):
Do not draw contours with less than `cut` number of points.
S : string or int
Resample smoothing factor.
l : str
Copy link
Member

Choose a reason for hiding this comment

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

grdcontour doesn't support -l option now. Currently only plot supports auto-label.

Copy link
Member

Choose a reason for hiding this comment

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

Resolving in #357.

@weiji14
Copy link
Member

weiji14 commented Oct 31, 2019

Oops, that's my bad 🤦‍♂️. I'll fix that.

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.

Support for legend with "handles" and "labels" like MATLAB/matplotlib
3 participants