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

Implement default position/box for legend #359

Merged
merged 6 commits into from
Nov 8, 2019
Merged

Implement default position/box for legend #359

merged 6 commits into from
Nov 8, 2019

Conversation

liamtoney
Copy link
Member

@liamtoney liamtoney commented Nov 1, 2019

Description of proposed changes

This sets the position argument of figure.legend() to default to JTR+jTR, i.e. upper-right inside of the plot. The motivation is convenience for quick plotting so folks don't have to remember alignment syntax etc.

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.

@vercel vercel bot temporarily deployed to staging November 1, 2019 08:04 Inactive
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.

I'd actually like to get the wider @GenericMappingTools/python team's opinion on this, since upstream GMT has a default auto-legend placement already and we're overriding it here . It would certainly be nice from a PyGMT user's perspective not to have to pass in anything to fig.legend().

I actually wonder if we can skip having to call fig.legend() altogether and have fig.show() return the automatic legend somehow. Not sure if there's a way to implement this here, or if it can be an upstream GMT change. Thoughts?

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

I actually wonder if we can skip having to call fig.legend() altogether and have fig.show() return the automatic legend somehow.

I am in favor of keeping the explicit call to fig.legend(), since it makes it more clear that a legend is desired.

@vercel vercel bot temporarily deployed to staging November 2, 2019 17:27 Inactive
@liamtoney
Copy link
Member Author

@weiji14 should I make a separate PR for 1e4a8a2 and b436ea4? This PR might take some time to discuss / implement.

@weiji14
Copy link
Member

weiji14 commented Nov 5, 2019

I am in favor of keeping the explicit call to fig.legend(), since it makes it more clear that a legend is desired.

Well the Zen of Python does have an "explicit is better than implicit" clause. Let's require users to call fig.legend() then, but we'll still need to find a way to implement that default position somehow without breaking short-letter-alias compatibility.

@weiji14 should I make a separate PR for 1e4a8a2 and b436ea4? This PR might take some time to discuss / implement.

Yes you're right, maybe move it to a separate PR. Will be a good time to learn how to git cherrypick!

@liamtoney
Copy link
Member Author

Ugh. I ran into the limits of my git / GitHub knowledge here trying to remove the commits from this branch that I cherry-picked for #371. Not sure what to do now...

@liamtoney
Copy link
Member Author

...and I lost my commit by merging with master. Yikes. I've deleted the now-useless branch. If this is something we want to re-open, let me know how to proceed. Sorry!

@weiji14
Copy link
Member

weiji14 commented Nov 6, 2019

Ouch, hang on there, I'll see if I can find a way to recover the deleted branch. There's actually a git command to do that but I'll hold off passing on advanced git lessons to you for now.

Edit: I take that back @liamtoney, try running git reflog to see if your can find your old commit/branch. I think it's b436ea4. I think you'll then need to run git checkout -b legend-default b436ea4b5a79f1cbd8f4ff4355e0a9b355cb1a7c.

References:

@liamtoney
Copy link
Member Author

The legend-default branch lives again! Thanks @weiji14 for your help.

@liamtoney
Copy link
Member Author

Okay, the implementation is more robust now I think.

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.

Good job on the branch recovery @liamtoney, you're definitely picking up things fast! Just a few more comments/questions.

pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Show resolved Hide resolved
Co-Authored-By: Wei Ji <23487320+weiji14@users.noreply.github.com>
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.

I think this looks good to go. Could you change the title to indicate that there's a default box setting as well? I'll merge this in after that, and maybe wait a few hours in case anyone has any last minute objections.

@liamtoney liamtoney changed the title Implement default position for legend Implement default position/box for legend Nov 8, 2019
@liamtoney
Copy link
Member Author

@weiji14 I think we're ready to merge!

@weiji14 weiji14 merged commit 49a7263 into GenericMappingTools:master Nov 8, 2019
@liamtoney liamtoney deleted the legend-default branch November 8, 2019 23:49
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.

2 participants