Potential fix for CRS.__eq__ #3208
Conversation
does away with to_epsg conversion and instead uses OSRIsSameEx to do the comparison with OGR. Might not be fast...
snowman2
left a comment
There was a problem hiding this comment.
I think that this is a great workaround. With the updated GDAL pins, this should be fine.
|
and just to confirm that this does seem to be much faster in the worst cases than it was before. Not as fast as directly comparing the wkt properties or the cached EPSG code, but I'd call a drop from the worst case of 20 ms to 5 us pretty good. If one wanted to be even more pedantic, I think you would call the current equality method an "nearly equal" due to the allowable changes in the coordinate axes and one would reserve the equality test to the more strict wkt comparison. In [25]: e = rio.CRS.from_epsg(4326)
In [26]: m = rio.CRS.from_user_input('IAU_2015:49900')
In [27]: e == m
Out[27]: False
In [28]: %timeit e.wkt == e.wkt
42.1 ns ± 0.0115 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [29]: %timeit e == e
5.14 μs ± 5.81 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
In [30]: %timeit m == m
5.03 μs ± 2.46 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
In [31]: m2 = rio.CRS.from_wkt('GEOGCS["Mars (2015) - Sphere XY",DATUM["Mars (2015) - Sphere",SPHEROID["Mars (2015) -
...: Sphere",3396190,0]],PRIMEM["Reference Meridian",0],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"
...: ]],AXIS["Longitude",EAST],AXIS["Latitude",NORTH]]')
In [32]: m2 == m
Out[32]: True
In [33]: %timeit m == m2
10.2 μs ± 6.3 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) |
sgillies
left a comment
There was a problem hiding this comment.
@AndrewAnnex thank you! I have a question/request inline.
* added test that demonstrates ogc:crs84 is equivalent to epsg:4326 for the purposes of rasterio
…t micro benchmarked
|
new equals method with default |
|
|
||
| wgs84_crs = CRS.from_string('+proj=longlat +ellps=WGS84 +datum=WGS84 +no_defs') | ||
| assert crs1 == wgs84_crs | ||
| assert crs1 != wgs84_crs |
There was a problem hiding this comment.
🤔 does this means the behaviour will change, or this is more a bug fix?
There was a problem hiding this comment.
that is a good question and is a question for the maintainers, I think the equality test before this PR was not working as intended. If the desire is for the axis order to not be ignored by default, then the behavior in the test above was a bug, because depending on the exact situation these proj4 strings could resolve as 4326 via to_epsg() and the equality check would first attempt to convert to epsg codes and match those. Seemingly the matches criteria is a bit looser by default when converting to epsg codes. For this test to keep that behavior one would use the new .equals method with the ignore flag enabled instead. If however we want to keep the "behavior" as is for this case and the other test, the ignore flag should be enabled by default, and to my eye epsg:4326 and crs84 are equivalent except for the axis order.
|
@sgillies in which version should we release this? do we have to wait for 1.5 or it could go into 1.4.2? @AndrewAnnex Could you add a note in the changelog 🙏 |
|
@vincentsarago there's a new method. Strictly speaking we can't add new methods in 1.4.2 if we're doing semantic versioning. I'm working on a backport for 1.4.2 that improves performance only. |
|
@sgillies I can also break the pr apart if needed to separate the new |
|
@vincentsarago still need me to add the change log entry for 1.5.0 or wait for the backport first? It doesn't look like rasterio follows the convention of including un-released versions with TBD dates in CHANGES.txt, so otherwise I'd have to add a line for 1.5.0 and 1.4.2... happy to do both/one/the other |
Additionally: stop asserting on "init=epsg:dddd" as this is no longer recommended usage.
|
@AndrewAnnex I'm going to merge this. The "backport" is a little more extensive than this PR as it eliminates more Don't worry about the change log. I'll take care of that when I merge 1.4.2 to main. |
* Eliminate from_user_inputs calls to prevent unintended recursion * Eliminate usage of to_epsg(), implement speed up from #3208 Additionally: stop asserting on "init=epsg:dddd" as this is no longer recommended usage. * Remove tests of equivalence with "+init=" CRS * Add EPSG:4326 vs OGC:CRS84 test. These are not equivalent.
first idea at fixing #3207
does away with to_epsg conversion and instead uses OSRIsSameEx to do the comparison with OGR.
However this might not be very fast, and rasterio doesn't seem to monitor benchmarks on ci tests unless I am missing something, will need to test locally to see if this is acceptable over comparing
wktparameters