Fix --boundary crash on fiona 1.10 (CRSError: Invalid PROJ string: EPSG:4326)#2040
Conversation
…SG:4326) fiona.crs.to_string() is deprecated in fiona 1.10 and returns "EPSG:4326" instead of a PROJ4 string. GeoJSON is always WGS84 (RFC 7946), so use CRS.from_epsg(4326) directly.
|
Additional confirmation that this is a fiona-version regression, not a code change —
In fiona 1.8.x, Since GeoJSON coordinates are always WGS84 by spec (RFC 7946), reading the source CRS back from the file is unnecessary, and |
|
Added a regression test in |
I would double check the spec. I'm afraid we cannot assume CRS of WGS84. The draft spec officially allowed it: And the final spec, strictly speaking, allows for it: Moreover, in practice, many prominent tools, like QGIS.org, will allow one to write to an alternative CRS. If this were a new feature, I would be really comfortable with requiring WGS84 CRS. But as this is an existing feature with possible use cases, we unfortunately probably need to continue to support other CRSs. |
Excellent! I have migrated that issue here so it can close on merge. It does make more sense here as a proper regression test. |
You're right, we shouldn't assume WGS 84. I updated the fix to keep reading
I also extended |
|
Yes. That's the better route. We don't need it to be backwards compatible, so best to use the newer API. |
smathermather
left a comment
There was a problem hiding this comment.
Code change looks great. @MJohnson459 -- can you double-check the tests? They look fine to me, but it would be better to get better eyes on them.
MJohnson459
left a comment
There was a problem hiding this comment.
Looks good apart from the duplicate test. Once that is removed I will accept.
| import unittest | ||
|
|
||
| from opendm.boundary import load_boundary | ||
|
|
||
|
|
||
| class TestBoundary(unittest.TestCase): | ||
| def setUp(self): | ||
| # A simple WGS84 (EPSG:4326) GeoJSON polygon | ||
| self.boundary = { | ||
| 'type': 'Feature', | ||
| 'properties': {'name': 'boundary'}, | ||
| 'geometry': { | ||
| 'type': 'Polygon', | ||
| 'coordinates': [[ | ||
| [-91.99544, 46.84260], | ||
| [-91.99417, 46.84260], | ||
| [-91.99417, 46.84337], | ||
| [-91.99544, 46.84337], | ||
| [-91.99544, 46.84260], | ||
| ]] | ||
| } | ||
| } | ||
|
|
||
| def test_load_boundary_without_reprojection(self): | ||
| # Without a target CRS the coordinates are returned unchanged | ||
| coords = load_boundary(self.boundary) | ||
| self.assertEqual(len(coords), 5) | ||
| self.assertAlmostEqual(coords[0][0], -91.99544, places=5) | ||
| self.assertAlmostEqual(coords[0][1], 46.84260, places=5) | ||
|
|
||
| def test_load_boundary_reprojects_to_utm(self): | ||
| # Regression test for https://github.com/OpenDroneMap/ODM/issues/2039 | ||
| # On fiona >= 1.10, fiona.crs.to_string() returns "EPSG:4326" instead of a | ||
| # PROJ4 string, which made CRS.from_proj4() raise | ||
| # "CRSError: Invalid PROJ string: EPSG:4326". GeoJSON is always WGS84 | ||
| # (RFC 7946), so load_boundary must reproject without raising. | ||
| utm15n = '+proj=utm +zone=15 +datum=WGS84 +units=m +no_defs' | ||
| coords = load_boundary(self.boundary, utm15n) | ||
| self.assertEqual(len(coords), 5) | ||
| # Reprojected coordinates are in meters (UTM), not degrees | ||
| for x, y in coords: | ||
| self.assertGreater(abs(x), 1000) | ||
| self.assertGreater(abs(y), 1000) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
There was a problem hiding this comment.
This seems to be a duplicate of the above?
There was a problem hiding this comment.
Removed the duplicated test block, thanks for catching it.
Removed duplicate unittest.main() call.
What
load_boundary()re-parsed the boundary's source CRS viaCRS.from_proj4(fiona.crs.to_string(src.crs)). On fiona 1.10,fiona.crs.to_string()is deprecated and returns the authority string"EPSG:4326"instead of a PROJ4 string, so pyproj'sCRS.from_proj4("EPSG:4326")raisesCRSError: Invalid PROJ string: EPSG:4326and any--boundaryrun aborts during the dataset stage.GeoJSON coordinates are WGS84 by specification (RFC 7946), so the source CRS is always EPSG:4326 and does not need to be read back from the file. This replaces the deprecated, broken call with an explicit
CRS.from_epsg(4326).Related
Ref #2039 (auto-closed by the bot, but this is a genuine reproducible bug — details and root cause are there).
Reproduction (before this PR)
Change
Verification
With the patch, the same input transforms correctly to UTM zone 35N (
669632.9, 4551824.3, …), and a full--boundaryrun completes past the dataset stage.Environment
ODM 3.6.0,
opendronemap/odm:gpu, fiona 1.10.1, pyproj 3.7.1.