Figure.grdview: Improve parameters plane/facade_fill/facade_pen to set the plane and facade #4235
+51
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seisman
added
enhancement
26 tasks
seisman
commented
Dec 3, 2025
Contributor
Summary of changed imagesThis is an auto-generated report of images that have changed on the DVC remote
Image diff(s)Added images
Modified images
Report last updated at commit 571b314 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Figure.grdview method by refactoring how plane and facade parameters are handled. It migrates facadepen to the new alias system and splits the GMT -N option into two separate parameters (plane and facadefill) for better API clarity.
Key changes:
- Split the
planeparameter into two distinct parameters:planefor z-level specification andfacadefillfor facade fill color - Migrated
facadepenfrom the old@use_aliasdecorator to the newAliasSystem - Added automatic plane enabling when
facadefillorfacadepenis specified
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pygmt/src/grdview.py | Updated function signature with new parameters, migrated to AliasSystem for -N and -Wf options, added auto-enable logic for plane |
| pygmt/tests/test_grdview.py | Updated existing test to use new API, added new test for facadepen with default plane |
| pygmt/tests/baseline/test_grdview_facadepen_default_plane.png.dvc | Added baseline image for new test case |
| examples/tutorials/advanced/draping_on_3d_surface.py | Updated examples to use new facadefill parameter instead of plane="+g..." syntax |
| examples/tutorials/advanced/3d_perspective_image.py | Updated examples and comments to use new facadefill parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
michaelgrund
approved these changes
Dec 4, 2025
seisman
added
final review call
Member
Author
|
Changes in this PR will affect our Figure 3, so ping @yvonnefroehlich for a review. |
seisman
removed
the
final review call
…e_pen, meshpen->mesh_pen (Will be removed in v0.20.0)
seisman
force-pushed
the
grdview/facade
branch
from
6d5264f to
82cd782
Compare
seisman
force-pushed
the
grdview/facade
branch
2 times, most recently
from
3a637e5 to
2c13607
Compare
…o set the plane and facade
seisman
force-pushed
the
grdview/facade
branch
from
2c13607 to
06ee20e
Compare
seisman
changed the title
Comment on lines
+113
to
+117
| plane | ||
| Draw a plane at the specified z-level. If ``True``, default to the minimum value | ||
| in the grid. However, if ``region`` was used to set zmin/zmax then that value is | ||
| used if it is less than the grid minimum value. Use ``facade_pen`` and | ||
| ``facade_fill`` to control the appearance of the plane. |
Member
There was a problem hiding this comment.
Suggested change
| plane | |
| Draw a plane at the specified z-level. If ``True``, default to the minimum value | |
| in the grid. However, if ``region`` was used to set zmin/zmax then that value is | |
| used if it is less than the grid minimum value. Use ``facade_pen`` and | |
| ``facade_fill`` to control the appearance of the plane. | |
| plane | |
| Draw a plane at the specified z-level. If ``True``, defaults to the minimum value | |
| in the grid. However, if ``region`` was used to set *zmin/zmax* then *zmin* is | |
| used if it is less than the grid minimum value. Use ``facade_pen`` and | |
| ``facade_fill`` to control the appearance of the plane. |
| data perimeter. | ||
| facade_pen | ||
| Set the pen attributes used for the facade. | ||
| shading : str |
Member
There was a problem hiding this comment.
Suggested change
| shading : str | |
| shading : str or float |
| # Set the plane elevation to 1,000 meters and make the fill "gray" | ||
| plane="1000+ggray", | ||
| plane=1000, # Set the plane elevation to 1,000 meters | ||
| facade_fill="gray", # Fill the facade with "gray" |
Member
There was a problem hiding this comment.
Suggested change
| facade_fill="gray", # Fill the facade with "gray" | |
| facade_fill="gray", # Color the facade in "gray" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits the
-Noption into two parameters,planefor setting the z-level andfacade_fillfor setting the fill for facade.Preview: https://pygmt-dev--4235.org.readthedocs.build/en/4235/api/generated/pygmt.Figure.grdview.html#pygmt.Figure.grdview
Related to #4208