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

Plot square or cube by default for geopandas Point/MultiPoint types #1405

Merged

Conversation

yohaimagen
Copy link
Contributor

In response to issue #1373 adding if statement to the plot function to create desirable default behavior for plotting geopandas dataframe with points data(geometry) to be plotted as points instead of a line.

Fixes #1373

@yohaimagen
Copy link
Contributor Author

I am not very happy with my solution.
first of all, as we discuss in #1373 it involves the plot function which has many many more utilities than just plotting geopandas DataFrams so it seems a bit too specific for this function.
second, I need to make sure that data is geopandas DataFrame before I am checking if it contains points only. In order to do so without importing geopandas I am checking that data contains the attribute _geometry_column_name "_geometry_column_name" in data.__dict__ by doing so I am assuming no other data option(string, np.ndarray etc...) will have this attribute. Another option will be to import geopands and to directly check that it is a geopandas DataFeame instant.

So if you have an idea for better implementation ill be happy to know.

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.

You've got the right idea here to check for an attribute that only geopandas.GeoDataFrame objects have 😄 I've offered a few suggestions that can help to improve things a bit.

Once we settle on the if-statement condition, it would be good to:

  1. copy the same chunk of code to plot3d.py.
  2. Add a unit test to test_geopandas.py to ensure this works. Are you interested in learning about unit test writing? If so, we can give you some tips on that.

P.S. I've converted this Pull Request (PR) to draft mode to save on Continuous Integration resources (test will only run on Linux instead of all 3 OS types). We will convert the PR back to non-draft mode once things are more polished.

pygmt/src/plot.py Outdated Show resolved Hide resolved
pygmt/src/plot.py Outdated Show resolved Hide resolved
pygmt/src/plot.py Show resolved Hide resolved
@weiji14 weiji14 added the enhancement Improving an existing feature label Aug 2, 2021
@weiji14 weiji14 marked this pull request as draft August 2, 2021 11:09
pygmt/src/plot.py Show resolved Hide resolved
pygmt/src/plot.py Outdated Show resolved Hide resolved
@yohaimagen
Copy link
Contributor Author

yohaimagen commented Aug 3, 2021

I am not sure about the p style. at least in my example, you almost can't see the 1-pixel points

from
fig.plot( projection='X10', frame='f', region=[0, 10, 0, 10, 0, 10], data=gdf, )
we get
plot
and from
fig.plot( projection='X10', frame='f', region=[0, 10, 0, 10, 0, 10], data=gdf, style='s0.2c' )
we get

plot_s0 2c

the same goes for plot3d with and without style='u0.2c'

plot3d
plot3d_u0 2c

@weiji14
Copy link
Member

weiji14 commented Aug 3, 2021

I am not sure about the p style. at least in my example, you almost can't see the 1-pixel points

Hmm, you're right. Let's go back to square 0.2cm then for plot, and perhaps we should use u0.2c for plot3d as you suggested. Sorry for the back and forth!

@yohaimagen
Copy link
Contributor Author

it's O.K.

now I'll be happy to learn about the tests. thanks!

@weiji14
Copy link
Member

weiji14 commented Aug 3, 2021

now I'll be happy to learn about the tests. thanks!

Alright! Testing involves checking that code produces the expected behaviour. Specifically, you'll need to write some new test functions to put in pygmt/tests/test_geopandas.py, and ensure that the function will generate an image that matches a baseline. The guidelines are at https://www.pygmt.org/dev/contributing.html#testing-plots.

The test itself can be adapted from what you did in #1405 (comment). You might also find the examples in test_plot.py useful. Without giving you too much of a full answer, it should look something like this:

@pytest.mark.mpl_image_compare
def test_geopandas_plot_default_square():
    """
    """
    point = shapely.geometry.Point(...)
    gdf = gpd.GeoDataFrame(...)
    fig = Figure()
    fig.plot(data=gdf, ...)
    return fig

Start with the above for a 2D plot with a Point type geometry, and once you've got the hang of it, try making a 3D plot with a MultiPoint type geometry. Good luck!

@yohaimagen
Copy link
Contributor Author

@weiji14 can you please add me as a collaborator at DAGshub.

Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dvc image diff test fails. Is that caused by the draft status or went something wrong with the upload @weiji14 ?

pygmt/tests/test_geopandas.py Outdated Show resolved Hide resolved
pygmt/tests/test_geopandas.py Show resolved Hide resolved
@maxrjones
Copy link
Member

The dvc image diff test fails. Is that caused by the draft status or went something wrong with the upload @weiji14 ?

dvc image failure is not a problem, see this post for an explanation #1405 (comment).

@seisman
Copy link
Member

seisman commented Aug 10, 2021

Please hold off merging this PR and see my comment in #1373 (comment) first.

@seisman
Copy link
Member

seisman commented Aug 11, 2021

I think we also need to add two tests to make sure users can plot the symbols using other styles when passing -Sc or others.

@yohaimagen
Copy link
Contributor Author

I'll add them

pygmt/src/plot.py Show resolved Hide resolved
pygmt/src/plot3d.py Show resolved Hide resolved
pygmt/src/plot.py Outdated Show resolved Hide resolved
pygmt/src/plot3d.py Outdated Show resolved Hide resolved
yohaimagen and others added 2 commits August 14, 2021 16:42
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@seisman seisman marked this pull request as ready for review August 14, 2021 19:48
@yohaimagen
Copy link
Contributor Author

yohaimagen commented Aug 15, 2021

great thanks for the review!

@seisman
Copy link
Member

seisman commented Aug 16, 2021

Ping @weiji14 and @michaelgrund to give this PR another round of final review, because this PR has changed a little after you approved it.

@weiji14
Copy link
Member

weiji14 commented Aug 16, 2021

I think it looks fine, just need to incorporate the suggestions at #1405 (comment) and should be good to merge 😄

@yohaimagen
Copy link
Contributor Author

Sorry did not notice them, committed the changes.

@weiji14
Copy link
Member

weiji14 commented Aug 17, 2021

Cool, I'll merge this PR in now, thanks again @yohaimagen!

@weiji14 weiji14 merged commit cd822ca into GenericMappingTools:main Aug 17, 2021
@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Aug 20, 2021
@yohaimagen yohaimagen deleted the geopandas-point-default-style branch August 24, 2021 05:38
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…enericMappingTools#1405)

Set default behavior for plotting geopandas dataframe with
Point/Multipoint type geometry as points (-Sp0.2c) for `plot`
and cubes (-Su0.2c) for `plot3d`, instead of GMT's default
line style plot.

* testing default behavior of plot and plot3d with geopandas Point and MultiPoint DataFrames
* adding test for non default style
* adding a comment explaining the if statment

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Michael Grund <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
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.

Plot point type geopandas.GeoDataFrames as points directly instead of as lines
5 participants