geopandas: Use io.StringIO to read geojson data and handle compatibility with geopandas v0.x and v1.x #3247
Conversation
seisman
added
maintenance
pygmt/helpers/tempfile.py
Outdated
| jsontext = json.dumps(geojson.__geo_interface__) | ||
| gpd.read_file(io.StringIO(jsontext)).to_file(**ogrgmt_kwargs) |
There was a problem hiding this comment.
Why is codecov reporting that these two lines are not covered by tests? Also, should we temporarily set engine='fiona' here to prevent ValueError: The 'schema' argument is not supported with the 'pyogrio' engine in the GMT Dev Tests?
| jsontext = json.dumps(geojson.__geo_interface__) | |
| gpd.read_file(io.StringIO(jsontext)).to_file(**ogrgmt_kwargs) | |
| jsontext = json.dumps(geojson.__geo_interface__) | |
| gpd.read_file(filename=io.StringIO(jsontext), engine="fiona").to_file(**ogrgmt_kwargs) |
There was a problem hiding this comment.
Also, should we temporarily set
engine='fiona'here to preventValueError: The 'schema' argument is not supported with the 'pyogrio' enginein the GMT Dev Tests?
It won't work. fiona is not installed in the GMT Dev Tests workflow, because it's no longer a required dependency for geopandas 1.x.
There was a problem hiding this comment.
Why is codecov reporting that these two lines are not covered by tests?
Not sure what happened, but now the coverage reports look good.
Edit:
it looks correct here https://app.codecov.io/gh/GenericMappingTools/pygmt/blob/geojson/pygmt%2Fhelpers%2Ftempfile.py
There was a problem hiding this comment.
Also, should we temporarily set
engine='fiona'here to preventValueError: The 'schema' argument is not supported with the 'pyogrio' enginein the GMT Dev Tests?It won't work.
fionais not installed in the GMT Dev Tests workflow, because it's no longer a required dependency for geopandas 1.x.
I meant to install fiona temporarily on GMT Dev Tests, so that the test pass for now with the change, but we'll need to look at how to handle this except-branch without fiona before PyGMT v0.13.0 is released. Also wondering if we should open an issue upstream to geopandas on what would be the recommended way forward if the 'schema' argument is not supported with pyogrio.
There was a problem hiding this comment.
It turns out pyogrio is "smarter" than fiona.
In #2497 (comment), I mentioned that
I thought world["pop_est_int"] = world.pop_est_round.astype("int32") should work, but it doesn't.
Changing the dtype directly doesn't work with fiona, but works well with pyorgio!.
In 2fb635b, I've added the if-test to check if it's geopandas v0.x or v1.x. For geopandas v0.x, we use the old codes which pass the updated schema parameter. For geopandas v1.x, we change the column dtype when necessary.
Now the "GMT Dev Tests" only have 5 failures that are unrelated to fiona/geopandas/pyogrio.
seisman
changed the title
| # The default engine "pyogrio" doesn't support the 'schema' parameter | ||
| # but we can change the dtype directly. | ||
| for col in geojson.columns: | ||
| if geojson[col].dtype in ("int", "int64", "Int64"): |
There was a problem hiding this comment.
Have to add "Int64" here because:
>>> import pandas as pd
>>> pd.Int32Dtype().name
'Int32'
>>> pd.Int64Dtype().name
'Int64'
Some new codes are reported to not covered by tests because these lines of codes are only executed for geopandas v1.x, which is not installed in our "Tests" workflow. |
seisman
removed
final review call
|
Thanks @seisman for getting this to work! Sorry that I haven't been responsive with the reviews, been travelling too much for conferences in the past few weeks. I'll spend some time this week to catch up. |
Description of proposed changes
__geo_interface__, read it viagdf.read_fileby wrapping the geojson object into aio.StringIOobjectfiona) and v1.x (default engine ispyogrio).Supersede #3237, so closes #3237.
This PR should fix the errors seen at #3180 (comment). So also closes #3231.