pygmt
pygmt copied to clipboard
Add clip parameter to pygmt.Figure.coast
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
andmake 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
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.
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, andclip="end"
ends the clip path. I prefer this overland="True"
andwater="True"
to start a clip path andend_clip="True"
to end the clip path.
Sounds like a perfect solution to me, thanks @meghanrjones!
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
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.
Btw, should we also use the static_earth_relief here?
Btw, should we also use the static_earth_relief here?
I think yes.
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?
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.
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.
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.
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?
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.
I saw the corresponding GMT PR was merged with only a fix in the docs @maxrjones. Do you think we can continue here?
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
)
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?
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?
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:
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