Skip to content

Comments

Potential fix for CRS.__eq__ #3208

Merged
sgillies merged 3 commits intorasterio:mainfrom
AndrewAnnex:speed_eq_up
Oct 22, 2024
Merged

Potential fix for CRS.__eq__ #3208
sgillies merged 3 commits intorasterio:mainfrom
AndrewAnnex:speed_eq_up

Conversation

@AndrewAnnex
Copy link
Contributor

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 wkt parameters

does away with to_epsg conversion and instead uses OSRIsSameEx to do the
comparison with OGR. Might not be fast...
Copy link
Member

@snowman2 snowman2 left a comment

Choose a reason for hiding this comment

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

I think that this is a great workaround. With the updated GDAL pins, this should be fine.

@snowman2 snowman2 added this to the 1.4.1 milestone Oct 11, 2024
@AndrewAnnex
Copy link
Contributor Author

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)

Copy link
Member

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

@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
@AndrewAnnex
Copy link
Contributor Author

new equals method with default ignore_axis_order=false while now ensuring CRS.from_epsg(4326) != CRS.from_string("OGC:CRS84") made it necessary to update two equality assertions between two tests


wgs84_crs = CRS.from_string('+proj=longlat +ellps=WGS84 +datum=WGS84 +no_defs')
assert crs1 == wgs84_crs
assert crs1 != wgs84_crs
Copy link
Member

Choose a reason for hiding this comment

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

🤔 does this means the behaviour will change, or this is more a bug fix?

Copy link
Contributor Author

@AndrewAnnex AndrewAnnex Oct 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgillies @snowman2, what do you think?

@AndrewAnnex AndrewAnnex requested a review from sgillies October 17, 2024 21:59
Copy link
Member

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

Thank you @AndrewAnnex !

@vincentsarago vincentsarago self-requested a review October 18, 2024 10:42
@vincentsarago
Copy link
Member

@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 🙏

@sgillies
Copy link
Member

@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.

@AndrewAnnex
Copy link
Contributor Author

@sgillies I can also break the pr apart if needed to separate the new .equals function. Or if you can add me as a co-author to the commit that's fine too

@AndrewAnnex
Copy link
Contributor Author

@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

sgillies added a commit that referenced this pull request Oct 20, 2024
Additionally: stop asserting on "init=epsg:dddd" as this is no
longer recommended usage.
@sgillies
Copy link
Member

sgillies commented Oct 20, 2024

@AndrewAnnex I'm going to merge this. The "backport" is a little more extensive than this PR as it eliminates more to_epsg() usage in the package and removes a bunch of test assertions about old PROJ4 "+init=x" usage.

Don't worry about the change log. I'll take care of that when I merge 1.4.2 to main.

@sgillies sgillies removed this from the 1.4.2 milestone Oct 22, 2024
@sgillies sgillies merged commit 74ccaf1 into rasterio:main Oct 22, 2024
sgillies added a commit that referenced this pull request Oct 22, 2024
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants