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 clip parameter to pygmt.Figure.coast #1706

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

Conversation

michaelgrund
Copy link
Member

@michaelgrund michaelgrund commented Jan 17, 2022

Description of proposed changes

This PR adds a clip parameter to the Figure.coast module:

  • clip="land" to clip dry areas
  • clip="water" to clip wet areas
  • clip ="end" to mark end of existing clip path

Fixes #

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • 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

This PR adds an alias for the ``-Q`` flag which I need to set up a new gallery example.
@michaelgrund michaelgrund added the documentation Improvements or additions to documentation label Jan 17, 2022
@michaelgrund michaelgrund added this to the 0.6.0 milestone Jan 17, 2022
@maxrjones
Copy link
Member

I like how GMT.jl handles clipping through the coast module (https://www.generic-mapping-tools.org/GMT.jl/dev/#GMT.coast) using a clip parameter that controls both starting and ending clip paths. In Python form, clip="land" starts clip path for land, clip="water" starts a clip path for water, and clip="end" ends the clip path. I prefer this over land="True" and water="True" to start a clip path and end_clip="True" to end the clip path.

@michaelgrund
Copy link
Member Author

I like how GMT.jl handles clipping through the coast module (https://www.generic-mapping-tools.org/GMT.jl/dev/#GMT.coast) using a clip parameter that controls both starting and ending clip paths. In Python form, clip="land" starts clip path for land, clip="water" starts a clip path for water, and clip="end" ends the clip path. I prefer this over land="True" and water="True" to start a clip path and end_clip="True" to end the clip path.

Sounds like a perfect solution to me, thanks @meghanrjones!

@michaelgrund michaelgrund marked this pull request as draft January 19, 2022 08:31
@michaelgrund michaelgrund marked this pull request as ready for review February 13, 2022 17:25
@michaelgrund michaelgrund changed the title Add missing Q alias to pygmt.Figure.coast Add clip parameter to pygmt.Figure.coast Feb 13, 2022
@michaelgrund michaelgrund requested review from willschlitzer and seisman and removed request for a team February 13, 2022 17:30
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.

Need to also add some unit tests to test_coast.py to cover the clip parameter. Maybe one for a land clip and one for a water clip.

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

seisman commented Feb 22, 2022

My one concern with the current implementation is that while backwards compatibility for Q=True is helpful for the creative users who've figured out that undocumented option, I imagine that people could try something like land=True expecting some default settings for plotting land masses and be confused when instead all their data are clipped at the land borders. One option around this could be to issue deprecation warnings for Q=True, land=True, or water=True, which could be handled in a separate PR.

I agree.

@maxrjones
Copy link
Member

Just a note regarding comment number 1 - the clip path applies to all plotting calls between the start of the clip path and the end of a clip bath. tbh I should have suggested a context manager for this case which would have been a more Pythonic way to manage clip="end".

@meghanrjones Do you still think we should use a context manager for clipping?

Possibly, I will look into this idea a bit more based the gmt clip module, which is very similar to these combinations of options to coast.

@maxrjones maxrjones mentioned this pull request Mar 1, 2022
9 tasks
@maxrjones
Copy link
Member

Just a note regarding comment number 1 - the clip path applies to all plotting calls between the start of the clip path and the end of a clip bath. tbh I should have suggested a context manager for this case which would have been a more Pythonic way to manage clip="end".

@meghanrjones Do you still think we should use a context manager for clipping?

Yes, after testing the feasibility with #1779, I still think that using a context manager for clipping land, water, and countries would be a good idea. @michaelgrund, I am sorry for not having my thoughts in order on the first review of this PR.

Here are the various options that I know of that set clipping paths in GMT:

  • clip (polygon clipping)
  • cloast -E+c|C (DCW i.e., geographic clipping)
  • coast -G (land clipping)
  • coast -S (water clipping)
  • mask (clip based on data coverage)

I think it would be better to have this functionality organized together in the PyGMT API rather than spread out amongst the other functions, but I'm not sure what the best way to accomplish that would be.

@michaelgrund
Copy link
Member Author

Yes, after testing the feasibility with #1779, I still think that using a context manager for clipping land, water, and countries would be a good idea. @michaelgrund, I am sorry for not having my thoughts in order on the first review of this PR.

Here are the various options that I know of that set clipping paths in GMT:

* clip (polygon clipping)

* cloast -E+c|C (DCW i.e., geographic clipping)

* coast -G (land clipping)

* coast -S (water clipping)

* mask (clip based on data coverage)

I think it would be better to have this functionality organized together in the PyGMT API rather than spread out amongst the other functions, but I'm not sure what the best way to accomplish that would be.

Totally agree with you @meghanrjones, should be bundled together. How should we continue with this PR?

@maxrjones
Copy link
Member

Totally agree with you @meghanrjones, should be bundled together. How should we continue with this PR?

I recommend leaving it as is briefly until we find out whether GMT can internally handle the -X -Y shifts when using clip paths (GenericMappingTools/gmt#6406) because the result from that discussion could impact our design of these functions.

@seisman seisman marked this pull request as draft March 5, 2022 05:32
@weiji14 weiji14 added enhancement Improving an existing feature and removed documentation Improvements or additions to documentation labels Mar 13, 2022
@weiji14 weiji14 modified the milestones: 0.6.0, 0.6.1 Mar 13, 2022
@seisman seisman modified the milestones: 0.6.1, 0.7.0 Mar 16, 2022
@seisman seisman modified the milestones: 0.7.0, 0.8.0 Jun 23, 2022
@michaelgrund
Copy link
Member Author

I saw the corresponding GMT PR was merged with only a fix in the docs @maxrjones. Do you think we can continue here?

@maxrjones
Copy link
Member

I saw the corresponding GMT PR was merged with only a fix in the docs @maxrjones. Do you think we can continue here?

Yes, continuing sounds good. Here's how I imagine the API looking with the various clip options being organized in a single namespace:

import pygmt
fig = pygmt.Figure()
with fig.clip.land():
    # Plot data with land clipped
with fig.clip.water():
    # Plot data with water clipped
with fig.clip.polygon(...):
    # Plot data with polygons clipped
with fig.clip.dcw(...):
    # Plot data with clipping based on dcw

Optionally, we could split up dcw, e.g., (fig.clip.country, fig.clip.continent, fig.clip.collection)

@seisman
Copy link
Member

seisman commented Sep 9, 2022

I saw the corresponding GMT PR was merged with only a fix in the docs @maxrjones. Do you think we can continue here?

Yes, continuing sounds good. Here's how I imagine the API looking with the various clip options being organized in a single namespace:

import pygmt
fig = pygmt.Figure()
with fig.clip.land():
    # Plot data with land clipped
with fig.clip.water():
    # Plot data with water clipped
with fig.clip.polygon(...):
    # Plot data with polygons clipped
with fig.clip.dcw(...):
    # Plot data with clipping based on dcw

Optionally, we could split up dcw, e.g., (fig.clip.country, fig.clip.continent, fig.clip.collection)

This looks like a good API design. If we decide to follow this API degisn, I think we should close this PR and then work in #1779 instead. Agree?

@weiji14
Copy link
Member

weiji14 commented Sep 9, 2022

Yes, continuing sounds good. Here's how I imagine the API looking with the various clip options being organized in a single namespace:

import pygmt
fig = pygmt.Figure()
with fig.clip.land():
    # Plot data with land clipped
with fig.clip.water():
    # Plot data with water clipped
with fig.clip.polygon(...):
    # Plot data with polygons clipped
with fig.clip.dcw(...):
    # Plot data with clipping based on dcw

Optionally, we could split up dcw, e.g., (fig.clip.country, fig.clip.continent, fig.clip.collection)

This looks like a good API design. If we decide to follow this API degisn, I think we should close this PR and then work in #1779 instead. Agree?

Is it possible, or would there be a need though to do a combined clip? E.g. some polygon + dcw?

@seisman
Copy link
Member

seisman commented Sep 9, 2022

Yes, continuing sounds good. Here's how I imagine the API looking with the various clip options being organized in a single namespace:

import pygmt
fig = pygmt.Figure()
with fig.clip.land():
    # Plot data with land clipped
with fig.clip.water():
    # Plot data with water clipped
with fig.clip.polygon(...):
    # Plot data with polygons clipped
with fig.clip.dcw(...):
    # Plot data with clipping based on dcw

Optionally, we could split up dcw, e.g., (fig.clip.country, fig.clip.continent, fig.clip.collection)

This looks like a good API design. If we decide to follow this API degisn, I think we should close this PR and then work in #1779 instead. Agree?

Is it possible, or would there be a need though to do a combined clip? E.g. some polygon + dcw?

That's a good question. GMT does support nested clipping paths:

gmt begin map
gmt coast -Rg -JH15c -W0.2p -Baf -Y40c
gmt coast -G
	gmt grdimage @earth_relief_01d
gmt coast -Q

gmt coast -Rg -JH15c -W0.2p -Baf -Y-8c
gmt coast -G
	gmt coast -ECN+c
		gmt grdimage @earth_relief_01d
	gmt coast -Q
gmt coast -Q

gmt coast -Rg -JH15c -W0.2p -Baf -Y-8c
gmt coast -G
	gmt clip <<- EOF
	110 30
	110 60
	180 60
	180 30
	EOF
		gmt grdimage @earth_relief_01d
	gmt clip -C
gmt coast -Q

gmt coast -Rg -JH15c -W0.2p -Baf -Y-8c
gmt coast -G
	gmt coast -ECN+c
		gmt clip <<- EOF
		110 30
		110 60
		180 60
		180 30
		EOF
		gmt grdimage @earth_relief_01d
		gmt clip -C
	gmt coast -Q
gmt coast -Q

gmt end show

The output is:
map

So nested clipping in PyGMT should be like:

with fig.clip.land():
    # Plot data with land clipped
    with fig.clip.polygon(...):
	    # Plot data with polygons clipped

@seisman seisman modified the milestones: 0.8.0, 0.9.0 Dec 11, 2022
@seisman seisman modified the milestones: 0.9.0, 0.10.0 Mar 19, 2023
@weiji14 weiji14 removed this from the 0.10.0 milestone Aug 25, 2023
@seisman seisman added this to the 0.12.0 milestone Dec 11, 2023
@seisman seisman removed this from the 0.12.0 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants