-
Notifications
You must be signed in to change notification settings - Fork 224
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 plot3d #471
Wrap plot3d #471
Conversation
Wrapping the `plot3d` function! Original GMT `plot3d` function can be found at https://docs.generic-mapping-tools.org/latest/plot3d.html. Current implementation is mostly copy-modify-pasted from `plot`, including the tests. Also modified added extra aliases to the `basemap` function to make it 3D capable.
Added new aliases (mostly based on GMT.jl's), and fixed old docstring to remove short-form aliases. Since `plot3d` and `plot` are similar, we might as well fix both of them at the same time. Also keeping the unit tests consistent.
Reorder aliases alphabetically and add common aliases verbose (V), xshift (X), yshift (Y) and transparency (t). Also changed to using aliases straight_line (A) and offset (D) in line with `plot`, and dropped error_bars (E) as it's not available in `plot3d`.
a0133cd
to
413ff77
Compare
See if this can resolve the Windows test failures
This reverts commit 413ff77.
@@ -26,6 +26,7 @@ Plotting data and laying out the map: | |||
Figure.coast | |||
Figure.colorbar | |||
Figure.plot | |||
Figure.plot3d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little off-topic, but should we sort all the methods alphabetically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just for the Figure.*
methods, but that's for a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
region = pygmt.info( | ||
table=df[["petal_width", "sepal_length", "petal_length"]], # x, y, z columns | ||
per_column=True, # report output as a numpy array | ||
spacing="1/2/0.5", # rounds x, y and z intervals by 1, 2 and 0.5 respectively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we should use spacing=[1, 2, 0.5]
here, but unfortunately, it's unimplemented in pygmt.info()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, forgot to handle that in #575. Should handle that (list inputs) when we complete the documentation for info
.
examples/gallery/plot/scatter3d.py
Outdated
|
||
# Make our 3D scatter plot, coloring each of the 3 species differently | ||
fig = pygmt.Figure() | ||
pygmt.makecpt(cmap="cubhelix", series=(0, 3, 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makecpt
command produces a discrete CPT (see the figure below). It doesn't make sense for the categorical data here.
.
Instead, we should use makecpt -F+c
, which produces categorical CPT.
But the colorbar annotations don't make sense to me. Perhaps another GMT bug.
Note: the gridlines are not shown in GMT 6.1.1. It's a new future in GMT master (see GenericMappingTools/gmt#3993), but the gridlines in XZ and YZ planes may also have bugs. Will report to GMT later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reported to upstream GenericMappingTools/gmt#4387 and GenericMappingTools/gmt#4388.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, we should use makecpt -F+c, which produces categorical CPT.
But the colorbar annotations don't make sense to me. Perhaps another GMT bug.
Ah yes, forgot about -F+c
! Actually, the Iris species categories are nominal (discrete) rather than ordinal (i.e. ranked 1st, 2nd, 3rd), so a legend would make more sense here than a colorbar. Another thought I had was to label each species using text
plotted in 3D, but we haven't implemented that 'feature' yet.
So for now, I think we can just use -F+c
here and ignore the colorbar, though we should add another categorical gallery example to close #244? I'd like to avoid adding a legend since it requires a for-loop to make the labels (and this complicates the example).
The wall frames look good though, too bad we need to wait for GMT 6.2.0! But good that we found two bugs to report to upstream GMT 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, makecpt -F
is not aliased yet. Do we want to do that here or in a separate PR?
Edit: Opened up #676 to complete documentation of makecpt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, with #676 merged, I've updated the code to use pygmt.makecpt(..., color_model="+c", ...)
pygmt/examples/gallery/plot/scatter3d.py
Line 34 in 40f7120
pygmt.makecpt(cmap="cubhelix", color_model="+c", series=(0, 3, 1)) |
In the future when we bump the minimum GMT version to 6.2.0 which would include the new feature at GenericMappingTools/gmt#4390, we can do the following:
pygmt.makecpt(cmap="cubhelix", color_model="+cSetosa,Versicolor,Virginica", series=(0, 3, 1))
fig.plot3d(...)
fig.colorbar()
fig.show()
which would produce a nice colorbar ;)
Co-Authored-By: Dongdong Tian <[email protected]>
cmap=True, # Use colormap created by makecpt | ||
region=region, # (xmin, xmax, ymin, ymax, zmin, zmax) | ||
frame=[ | ||
"WsNeZ3", # z axis label positioned on 3rd corner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding a title here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
Co-authored-by: Dongdong Tian <[email protected]>
Thanks for the thorough review @seisman! |
Wrapping the
plot3d
function! Original GMTplot3d
function can be found at https://docs.generic-mapping-tools.org/latest/plot3d.html. Current implementation is mostly copy-modify-pasted fromplot
, including the tests. Also added extra aliases to thebasemap
function to make it 3D capable.Live documentation preview of
plot3d
API is at https://pygmt-git-mapping-plot3d.gmt.vercel.app/api/generated/pygmt.Figure.plot3d.html. Gallery example is at https://pygmt-git-mapping-plot3d.gmt.vercel.app/gallery/plot/scatter3d.htmlDescription of proposed changes
This is the gallery example of a 3D plot with the Iris flower dataset, inspired by the example at https://scikit-learn.org/stable/auto_examples/cluster/plot_cluster_iris.html 😄
Here's another example with a bit of a geographic curve:
produces:
Alternatively, we could follow GMT.jl's example at https://www.generic-mapping-tools.org/GMT.jl/latest/plot/ and just have one
plot
function, which would handle 3D plotting if thez
dimension is provided. Thoughts?Fixes #467
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.