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

Fix UnicodeDecodeError with shapefiles for plot and plot3d #1695

Merged
merged 14 commits into from
Mar 1, 2022

Conversation

seisman
Copy link
Member

@seisman seisman commented Jan 6, 2022

Description of proposed changes

Fixes #1616

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

@seisman seisman added the bug Something isn't working label Jan 6, 2022
@seisman seisman added this to the 0.6.0 milestone Jan 6, 2022
@seisman
Copy link
Member Author

seisman commented Jan 7, 2022

Do I need to add a test for #1616? If yes, is there a shapefile that I can use?

@maxrjones
Copy link
Member

Do I need to add a test for #1616? If yes, is there a shapefile that I can use?

I think it would be good to have either a test or a gallery example.

One option would be to use the same data as the roads gallery example with gdf.to_file. Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and https://github.com/GenericMappingTools/gmt/blob/master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

@seisman
Copy link
Member Author

seisman commented Jan 9, 2022

One option would be to use the same data as the roads gallery example with gdf.to_file. Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and GenericMappingTools/gmt@master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

Sounds a good idea.

@seisman
Copy link
Member Author

seisman commented Jan 10, 2022

One option would be to use the same data as the roads gallery example with gdf.to_file. Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and GenericMappingTools/gmt@master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

Sounds a good idea.

Unfortunately it won't work, because the ogr2ogr command produces four files (.shp, .shx, .dbf and .prj), and the GMT cache mechanism cannot download 4 files using @RidgeTest.shp.

@weiji14
Copy link
Member

weiji14 commented Jan 10, 2022

One option would be to use the same data as the roads gallery example with gdf.to_file. Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and GenericMappingTools/gmt@master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

Sounds a good idea.

Unfortunately it won't work, because the ogr2ogr command produces four files (.shp, .shx, .dbf and .prj), and the GMT cache mechanism cannot download 4 files using @RidgeTest.shp.

You could do gmt which -Gl @RidgeTest.shp @RidgeTest.shx @RidgeTest.dbf @RidgeTest.prj 🙂

@weiji14
Copy link
Member

weiji14 commented Jan 13, 2022

There's also some sample shapefile data at https://github.com/intake/intake_geopandas/tree/0.4.0/tests/data/stations you can just download and use.

@willschlitzer
Copy link
Contributor

One option would be to use the same data as the roads gallery example with gdf.to_file. Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and GenericMappingTools/gmt@master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

Sounds a good idea.

Unfortunately it won't work, because the ogr2ogr command produces four files (.shp, .shx, .dbf and .prj), and the GMT cache mechanism cannot download 4 files using @RidgeTest.shp.

Could the .shp file be added to pygmt/tests/data as a workaround to have a cached file?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2022

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_plot_shapefile.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_plot_shapefile.png

Modified images

Path Old New

Report last updated at commit 3712edc

@@ -0,0 +1 @@
GEOGCS["GCS_WGS_1984",DATUM["D_WGS_1984",SPHEROID["WGS_1984",6378137.0,298.257223563]],PRIMEM["Greenwich",0.0],UNIT["Degree",0.0174532925199433]]
Copy link
Member

@weiji14 weiji14 Feb 24, 2022

Choose a reason for hiding this comment

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

Still not sure if storing the shapefile (shp/shx/dbf/prj) in the git history is a good idea. I'm thinking whether we should put it on dvc, or maybe add some custom code to pygmt.helpers.testing.download_test_data() to download the shapefile using urllib or plain pygmt.which.

from pygmt import which

for ext in ["shp", "shx", "dbf", "prj"]:
    which(
        fname=f"https://github.com/intake/intake_geopandas/raw/0.4.0/tests/data/stations/stations.{ext}",
        download=True,
    )

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option would be to convert RidgeTest.gmt to a shapefile (possibly we could cache the shapefile to speed up the new PyGMT test and https://github.com/GenericMappingTools/gmt/blob/master/test/psxy/categorical.sh):

test_data=`gmt which -Gl @RidgeTest.gmt`
ogr2ogr -f "ESRI Shapefile" RidgeTest.shp $test_data

I'm considering the option from @meghanrjones again. I think it makes more sense to cache the shapefile in the GMT server, because they can also be used by GMT and GMT.jl.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Thanks @seisman! Looks nice and clean now.

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Feb 27, 2022
@seisman
Copy link
Member Author

seisman commented Feb 28, 2022

/format

@seisman seisman marked this pull request as draft February 28, 2022 15:17
@seisman
Copy link
Member Author

seisman commented Feb 28, 2022

It seems the new test crashes.

@maxrjones
Copy link
Member

It seems the new test crashes.

I do not see the new RidgeTest.shp files in the data downloaded from the cache. You may need to rerun the cache data workflow.

@seisman
Copy link
Member Author

seisman commented Mar 1, 2022

It seems the new test crashes.

I do not see the new RidgeTest.shp files in the data downloaded from the cache. You may need to rerun the cache data workflow.

You're right, there is a scheduled run of the cache data workflow yesterday.

@seisman seisman marked this pull request as ready for review March 1, 2022 01:09
@seisman seisman merged commit 13065b3 into main Mar 1, 2022
@seisman seisman deleted the ogrgmt-detection branch March 1, 2022 01:31
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pygmt v0.5.0 UnicodeDecodeError with fig.plot() when plotting shapefiles
4 participants