pygmt icon indicating copy to clipboard operation
pygmt copied to clipboard

Add clip parameter to pygmt.Figure.coast

Open michaelgrund opened this issue 3 years ago • 17 comments

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

michaelgrund avatar Jan 17 '22 07:01 michaelgrund

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.

maxrjones avatar Jan 18 '22 15:01 maxrjones

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 avatar Jan 18 '22 16:01 michaelgrund

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_coast_clip_land.png
added pygmt/tests/baseline/test_coast_clip_water.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_coast_clip_land.png

  • pygmt/tests/baseline/test_coast_clip_water.png

Modified images

Path Old New

Report last updated at commit 98dde53a145f8e687b207fa559494d239dfecf7d

github-actions[bot] avatar Feb 15 '22 22:02 github-actions[bot]

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.

Tests are added @weiji14.

michaelgrund avatar Feb 15 '22 22:02 michaelgrund

Btw, should we also use the static_earth_relief here?

michaelgrund avatar Feb 16 '22 08:02 michaelgrund

Btw, should we also use the static_earth_relief here?

I think yes.

seisman avatar Feb 16 '22 12:02 seisman

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?

seisman avatar Feb 22 '22 13:02 seisman

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.

seisman avatar Feb 22 '22 13:02 seisman

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 avatar Mar 01 '22 17:03 maxrjones

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 https://github.com/GenericMappingTools/pygmt/pull/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.

maxrjones avatar Mar 01 '22 23:03 maxrjones

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?

michaelgrund avatar Mar 02 '22 07:03 michaelgrund

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 (https://github.com/GenericMappingTools/gmt/issues/6406) because the result from that discussion could impact our design of these functions.

maxrjones avatar Mar 03 '22 15:03 maxrjones

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

michaelgrund avatar Aug 31 '22 06:08 michaelgrund

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)

maxrjones avatar Aug 31 '22 19:08 maxrjones

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 https://github.com/GenericMappingTools/pygmt/pull/1779 instead. Agree?

seisman avatar Sep 09 '22 15:09 seisman

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?

weiji14 avatar Sep 09 '22 15:09 weiji14

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 avatar Sep 09 '22 15:09 seisman