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

Add tutorial for pygmt.Figure.text #480

Merged
merged 10 commits into from
Oct 15, 2020

Conversation

hemmelig
Copy link
Contributor

Add tutorial covering the basics of text annotation with PyGMT.
Follows the same format as the existing tutorials. This tutorial
covers the currently available suite of arguments in the PyGMT binding
to the GMT 6 program 'text'.

Fixes #469

Add tutorial detailing how to add text annotations to PyGMT figures. Follows
the style of the other tutorials available, showing how to use the currently
available arguments in the binding to the underlying GMT program 'text'.

Fixes GenericMappingTools#469
@welcome
Copy link

welcome bot commented Jun 18, 2020

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

  • If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
  • All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
  • No matter what, we are really grateful that you put in the effort to do this! 🎉

examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
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 @hemmelig, really great to see this example! I actually grew up on Borneo so I guess I'll need to review this properly 😆 You'll notice that we use Continuous Documentation #344, so you can preview what it will look like at https://pygmt-git-fork-hemmelig-texttutorial.gmt.vercel.app/tutorials/text.html every time a commit is made. Here's a few things I spotted:

  • Links in rst need a double-underscore at the end
  • You'll need to run make lint to get rid of the stickler.yml messages

Will recommend some suggested changes in a bit, it's great to have it in the PR so we can discuss things line by line.

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.

Oh and by the way, if you want to preview the documentation locally, you do so following the instructions at https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#documentation. Basically:

cd doc
make all
make serve


fig = pygmt.Figure()
fig.basemap(region=[0, 1, 0, 1], projection="X5c", frame="WSen")
fig.text(text="Green", x=0.5, y=0.5, G="green")
Copy link
Member

Choose a reason for hiding this comment

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

We're actually trying to document only the long-form arguments (see #473), and have made a start in #474. Just that we haven't released it yet, see differences in documentation between https://www.pygmt.org/dev/api/generated/pygmt.Figure.text.html and https://www.pygmt.org/v0.1.1/api/generated/pygmt.Figure.text.html#pygmt.Figure.text. I've been thinking whether we should cut a v0.1.2 release to incorporate this PR and other documentation changes, before GMT 6.1.0 comes out around end of June.

Personally I have nothing against using short aliases like this to achieve additional functionality (since I do it too!). Better way would be to add an alias for G=color here, see #385 for an example on how to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was planning to collect some thoughts on how best to bring the rest of the available flags, or short-form arguments, into the PyGMT fold for text and submit an issue.

So for now, should I remove this last section from the tutorial?

Copy link
Member

Choose a reason for hiding this comment

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

Or we could do a quick PR to get some extra aliases into text. They're basically one-line additions (per alias). Do you have a list of what you want besides color?

Copy link
Contributor Author

@hemmelig hemmelig Jun 18, 2020

Choose a reason for hiding this comment

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

It would make sense to also add an alias for W (pen?) if adding one for G.

I find having font as an alias for F when +f also relates to font a bit ambiguous. There was also the point I raised in Issue #479 - when adding text from a text file, it is unclear if it's actually possible to emulate the way you tell GMT to expect font, angle, or justify fields in the file (-F+f+a+j).

Copy link
Member

@weiji14 weiji14 Jun 18, 2020

Choose a reason for hiding this comment

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

It would make sense to also add an alias for W (pen?) if adding one for G.

Yes, W=pen seems like another obvious choice.

I find having font as an alias for F when +f also relates to font a bit ambiguous. There was also the point I raised in Issue #479 - when adding text from a text file, it is unclear if it's actually possible to emulate the way you tell GMT to expect font, angle, or justify fields in the file (-F+f+a+j).

Looking at the current v0.1.1 text implementation

if angle is not None or font is not None or justify is not None:
if "F" not in kwargs.keys():
kwargs.update({"F": ""})
if angle is not None and isinstance(angle, (int, float)):
kwargs["F"] += f"+a{str(angle)}"
if font is not None and isinstance(font, str):
kwargs["F"] += f"+f{font}"
if justify is not None and isinstance(justify, str):
kwargs["F"] += f"+j{justify}"

I think if you want to emulate -F+f+a+j, you could do fig.text(..., font=True, angle=True, justify=True), and that would then use treat the columns in the textfile as font/angle/justify respectively. Unfortunately it won't be possible to change the order of the columns (e.g. -F+a+j+f I think.

But yes, you're right that it's an ambiguous beast. We could either document this particular case for GMT 'power' users, or find a better way to implement the code block above, bearing in mind that PyGMT is meant to be accessible to new users (see Project Goals) who might not be as familiar with the GMT suite. It would be nice to somewhat emulate what matplotlib.pyplot.text does for example (i.e. have a fontdict like argument).

Copy link
Member

Choose a reason for hiding this comment

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

Yes - works for justify and font, but wouldn't work for angle. Ideally though you would want to be able to set them in the same way?

True, should allow str type for angle as well. I'll start a Pull Request to add some enhancements to text, give me a few minutes.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've started PR #481 to handle this, so angle="" should work once it's merged in to master. Also checked that angle=True, font=True and justify=True works (See test case at https://github.com/GenericMappingTools/pygmt/pull/481/files#diff-c20c076528e6988bc916b4bfc47e7a8d.
). The documentation at https://www.pygmt.org/dev/api/generated/pygmt.Figure.text does mention that bool types are allowed. That would translate directly to -F+a+f+j to GMT.

Copy link
Contributor Author

@hemmelig hemmelig Jun 20, 2020

Choose a reason for hiding this comment

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

In which case, perhaps it would be best to wait until the above has been merged, so we can give the example as:

fig = pygmt.Figure()
with pygmt.config(MAP_FRAME_TYPE="plain"):
    fig.basemap(region=[108, 120, -5, 8], projection="M20c", frame="a")
fig.coast(land="black", water="skyblue")

# Create file
with open("examples.txt", "w") as f:
    f.write("114 0.5 0 22p,Helvetica-Bold,white CM BORNEO\n")
    f.write("119 3.25 0 12p,Helvetica-Bold,black CM CELEBES SEA\n")
    f.write("112 -4.6 0 12p,Helvetica-Bold,black CM JAVA SEA\n")
    f.write("112 6 40 12p,Helvetica-Bold,black CM SOUTH CHINA SEA\n")
    f.write("119.12 7.25 -40 12p,Helvetica-Bold,black CM SULU SEA\n")
    f.write("118.4 -1 65 12p,Helvetica-Bold,black CM MAKASSAR STRAIT\n")

# Plot region names / sea names
fig.text(textfiles="examples.txt", angle=True, font=True, justify=True)

# Cleanup
os.remove("examples.txt")

fig.show()

Copy link
Member

@weiji14 weiji14 Jun 21, 2020

Choose a reason for hiding this comment

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

Ok, we've merged in #481! If you can merge in the new changes from master into this branch (e.g. by clicking on the 'Update Branch' button), we can start to simplify some of the example code. The new text features/enhancements should be documented at https://www.pygmt.org/dev/api/generated/pygmt.Figure.text.html#pygmt.Figure.text (once the CI build finishes). Let us know if you need any help.

Copy link
Member

@weiji14 weiji14 Oct 14, 2020

Choose a reason for hiding this comment

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

The example here with using with open("examples.txt", "w") as f: ... might be another reason to support textfile=io.StringIO input as with #576, just so we don't need to explicitly create a temporary .txt file. But let's do this another time 😉 Edit: done at commit 20994aa.

So for now, should I remove this last section from the tutorial?

Done at commit 3dde4b0, and I've also kept the section but am using fill instead of G now.

examples/tutorials/text.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added the documentation Improvements or additions to documentation label Jun 18, 2020
examples/tutorials/text.py Outdated Show resolved Hide resolved
weiji14 added a commit that referenced this pull request Jun 21, 2020
Adding extra functionality to text, including aliases that were missing in initial implementation at #321, in support of the text tutorial at #480. Allow passing str type into angle argument of text. Aliased pen(W), clearance(C), fill(G), and offset(D). Enable text placement using position (`-F+c`) argument instead of x/y pairs. Reorder docstring to say 'angle, font, justify' instead of 'font, angle, justify'. Added test_text_position to plot at all 9 possible positions. Removed check for missing file in textfiles, and checked that plotting text from an external file (@Table_5_11.txt) works.

Co-authored-by: Dongdong Tian <[email protected]>
@weiji14 weiji14 mentioned this pull request Jul 2, 2020
10 tasks
@weiji14 weiji14 mentioned this pull request Sep 6, 2020
11 tasks
@seisman
Copy link
Member

seisman commented Sep 19, 2020

@hemmelig Do you still have time working on this PR? These are all good text examples and should be merged into the documentation.

@seisman
Copy link
Member

seisman commented Sep 26, 2020

No response from @hemmelig for 7 days. I'll take over this PR.

@hemmelig
Copy link
Contributor Author

What exactly needs finishing? I have been on fieldwork and moving home, apologies.

@seisman
Copy link
Member

seisman commented Sep 26, 2020

@hemmelig No apologies. The code style checking is still failing (you should be able to run make format to update the code style automatically), and there are still a few review comments above that should be addressed.

@seisman
Copy link
Member

seisman commented Sep 26, 2020

Other tests are also failing. It seems we introduced a bug in v0.2.0.

examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Oct 14, 2020

I'll work on finalizing this @seisman, shouldn't let a good tutorial example sit here for so long!

examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
Comment on lines +25 to +26
with pygmt.config(MAP_FRAME_TYPE="plain"):
fig.basemap(region=[108, 120, -5, 8], projection="M20c", frame="a")
Copy link
Member

Choose a reason for hiding this comment

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

As an introductory tutorial, we probably should avoid using unrelated methods pygmt.config(), although plain frames sometimes look better than "fancy" frames. But I'm also OK to keep pygmt.config() here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to keep it here for now, this might reduce the number of 'fancy' frames out there in the world 😆

examples/tutorials/text.py Outdated Show resolved Hide resolved
Comment on lines +48 to +49
with pygmt.config(MAP_FRAME_TYPE="plain"):
fig.basemap(region=[108, 120, -5, 8], projection="M20c", frame="a")
Copy link
Member

Choose a reason for hiding this comment

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

Same as above for pygmt.config().

Comment on lines +65 to +66
with pygmt.config(MAP_FRAME_TYPE="plain"):
fig.basemap(region=[108, 120, -5, 8], projection="M20c", frame="a")
Copy link
Member

Choose a reason for hiding this comment

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

Same as above for pygmt.config().

examples/tutorials/text.py Outdated Show resolved Hide resolved
examples/tutorials/text.py Outdated Show resolved Hide resolved
@seisman seisman added this to In progress in Release v0.2.x via automation Oct 15, 2020
@seisman seisman added this to the 0.2.1 milestone Oct 15, 2020
@weiji14 weiji14 merged commit 0453ea0 into GenericMappingTools:master Oct 15, 2020
Release v0.2.x automation moved this from In progress to Done Oct 15, 2020
@welcome
Copy link

welcome bot commented Oct 15, 2020

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@weiji14
Copy link
Member

weiji14 commented Oct 15, 2020

Thanks for working on this @hemmelig!! The new documentation is now at https://www.pygmt.org/dev/tutorials/text.html 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Need examples for adding texts on maps
4 participants