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 ternary #1431

Merged
merged 33 commits into from
Sep 16, 2022
Merged

Wrap ternary #1431

merged 33 commits into from
Sep 16, 2022

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Aug 10, 2021

This pull request wraps the GMT function ternary.

Preview docs at https://pygmt-git-wrap-ternary-gmt.vercel.app/api/generated/pygmt.Figure.ternary.html

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.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@willschlitzer willschlitzer added the feature Brand new feature label Aug 10, 2021
@willschlitzer willschlitzer added this to the 0.5.0 milestone Aug 10, 2021
@willschlitzer willschlitzer self-assigned this Aug 10, 2021
@willschlitzer
Copy link
Contributor Author

I'm getting a weird problem where the axis labels on the right-side (b) axis isn't showing up. I'm not sure what I'm missing, as this follows along with the example on the GMT page for ternary, and there are still gridlines for that axis appearing in the final figure.

fig = pygmt.Figure()
pygmt.makecpt(cmap="turbo", series=[0, 80, 10])
fig.ternary(table="@ternary.txt",
            region="0/100/0/100/0/100",
            cmap=True,
            projection="X6i",
            frame= ["bafg+lAir",
                   "cafg+lLimestone",
                   "aafg+lWater"],
            S="c0.1c")
fig.show()

image

@maxrjones
Copy link
Member

I'm getting a weird problem where the axis labels on the right-side (b) axis isn't showing up. I'm not sure what I'm missing, as this follows along with the example on the GMT page for ternary, and there are still gridlines for that axis appearing in the final figure.

fig = pygmt.Figure()
pygmt.makecpt(cmap="turbo", series=[0, 80, 10])
fig.ternary(table="@ternary.txt",
            region="0/100/0/100/0/100",
            cmap=True,
            projection="X6i",
            frame= ["bafg+lAir",
                   "cafg+lLimestone",
                   "aafg+lWater"],
            S="c0.1c")
fig.show()

image

This was a bug in GMT in 6.2.0, which was fixed by GenericMappingTools/gmt#5431.

@willschlitzer
Copy link
Contributor Author

This was a bug in GMT in 6.2.0, which was fixed by GenericMappingTools/gmt#5431.

@meghanrjones Is there a workaround for GMT 6.2.0, or will this plotting bug be the case until GMT releases a new version and PyGMT uses it?

@maxrjones
Copy link
Member

maxrjones commented Aug 11, 2021

This was a bug in GMT in 6.2.0, which was fixed by GenericMappingTools/gmt#5431.

@meghanrjones Is there a workaround for GMT 6.2.0, or will this plotting bug be the case until GMT releases a new version and PyGMT uses it?

You could try using pygmt.config(MAP_FRAME_AXIS, "WSE"), though using that internally would prevent users from overriding it unless you found a way to check whether the setting has been configured in the session.

Edit: To be more clear, it would need to be in a context manager for pygmt.ternary calls so that it doesn't impact other function calls.

@willschlitzer
Copy link
Contributor Author

You could try using pygmt.config(MAP_FRAME_AXIS, "WSE"), though using that internally would prevent users from overriding it unless you found a way to check whether the setting has been configured in the session.

Edit: To be more clear, it would need to be in a context manager for pygmt.ternary calls so that it doesn't impact other function calls.

I think calling ternary with a context manager will cause more confusion when none of the other plotting functions do, especially since this will presumably go back to normal in the next release of GMT (which I'm assuming will be after PyGMT v0.5.0 release). My thought is to just hold off on merging the upstream fix takes effect, as there are other methods to make ternary plots with Python.

@weiji14 weiji14 modified the milestones: 0.5.0, 0.6.0 Sep 30, 2021
@weiji14
Copy link
Member

weiji14 commented Dec 15, 2021

Since we've bumped PyGMT's minimum GMT version to 6.3.0, do you want to give this PR another go @willschlitzer?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2021

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
added pygmt/tests/baseline/test_ternary.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_ternary.png

Modified images

Path Old New

Report last updated at commit 33b200e

@willschlitzer willschlitzer changed the title WIP: Wrap ternary Wrap ternary Dec 21, 2021
@willschlitzer willschlitzer marked this pull request as ready for review December 21, 2021 19:55
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.

First round of review. Implementation look ok, but the docs could use a bit of work. Could you please add ternary to doc/api/index.rst so that we can see the preview docs? Thanks!

pygmt/src/ternary.py Outdated Show resolved Hide resolved
pygmt/src/ternary.py Outdated Show resolved Hide resolved
pygmt/tests/test_ternary.py Outdated Show resolved Hide resolved
pygmt/tests/test_ternary.py Outdated Show resolved Hide resolved
@maxrjones
Copy link
Member

Hi @willschlitzer, I think this PR is good to merge after two small in-line suggestions (one for a line length issue to satisfy pylint and one to add a reference to plotting styles). Would you be open to moving forward with a merge rather than closing the PR?

@willschlitzer
Copy link
Contributor Author

@meghanrjones Not sure how I managed this (currently on a work computer), but didn't mean to close it! Thanks

@willschlitzer willschlitzer reopened this May 3, 2022
pygmt/src/ternary.py Outdated Show resolved Hide resolved
counter-clockwise direction].
{R}
{CPT}
{G}
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 we can ignore it now and address the issue in #1617 instead.

pygmt/src/ternary.py Outdated Show resolved Hide resolved
@seisman seisman modified the milestones: 0.7.0, 0.8.0 Jun 23, 2022
@seisman
Copy link
Member

seisman commented Aug 27, 2022

@willschlitzer I think this PR is almost ready for merge. I'm wondering if you have some time to finalize this PR recently.

@willschlitzer willschlitzer marked this pull request as ready for review September 12, 2022 13:18
@willschlitzer
Copy link
Contributor Author

/format

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

seisman commented Sep 13, 2022

The tests fail, likely due to the outdated baseline image. Could you please regenerate the baseline image using GMT 6.4.0 again?

@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed final review call This PR requires final review and approval from a second reviewer labels Sep 14, 2022
@seisman seisman merged commit 561eb41 into main Sep 16, 2022
@seisman seisman deleted the wrap-ternary branch September 16, 2022 00:40
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants