Skip to content

Comments

adds IAU as known authority to CRS.to_authority#3206

Merged
sgillies merged 5 commits intorasterio:mainfrom
AndrewAnnex:add_iau
Nov 8, 2024
Merged

adds IAU as known authority to CRS.to_authority#3206
sgillies merged 5 commits intorasterio:mainfrom
AndrewAnnex:add_iau

Conversation

@AndrewAnnex
Copy link
Contributor

also adds a test, recent versions of Proj include the IAU CRSs so this should work as is

@AndrewAnnex
Copy link
Contributor Author

hmm is there some place where the GDAL/Proj versions are listed? This should work with gdal versions from last year

@vincentsarago
Copy link
Member

recent versions of Proj include the IAU CRSs so this should work as is

@AndrewAnnex do you know which version ?

@AndrewAnnex
Copy link
Contributor Author

AndrewAnnex commented Oct 10, 2024

@vincentsarago I think the very first version was 8.2.0 based on OSGeo/PROJ#2876, but maybe 9.2.1 would be a bit more up to date as a minimum lower version bound OSGeo/PROJ#3704

@vincentsarago
Copy link
Member

tests are failing because CRS.from_user_input('IAU_2015:49900') returns None

which is weird because this works locally with the latest 1.4 rasterio wheels 🤷‍♂️

CRS.from_user_input('IAU_2015:49900')
>>> CRS.from_wkt('GEOGCS["Mars (2015) - Sphere / Ocentric",DATUM["Mars (2015) - Sphere",SPHEROID["Mars (2015) - Sphere",3396190,0,AUTHORITY["IAU","49900"]],AUTHORITY["IAU","49900"]],PRIMEM["Reference Meridian",0,AUTHORITY["IAU","49900"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AXIS["Latitude",NORTH],AXIS["Longitude",EAST],AUTHORITY["IAU","49900"]]')

@AndrewAnnex
Copy link
Contributor Author

sort-of lost track of this pr, I was thinking that maybe the CI builds have outdated versions of the Proj data directory (I see mentions of a 9.2.0 proj docker container)

@AndrewAnnex
Copy link
Contributor Author

AndrewAnnex commented Oct 22, 2024

I looked into this a bit more, going so far as to dump the proj.db file in the docker image for gdal small and I see the IAU codes in there, but it is a little hard to know from the various files and docker images used by the CI here what proj version is being used when. I'll try again with the larger proj image next but given that it is 9.2.0 version I'd suspect it'd be fine (returning to this later, it was fine, I see all the expected codes)

return "OGC", matches["OGC"][0]
elif "ESRI" in matches:
return "ESRI", matches["ESRI"][0]
elif "IAU_2015" in matches:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like the version for the IAU authority is incorporated into the name? I had forgotten this was the case and I've seen it before in other places, but I wonder if it makes sense to do something more complicated here like a any(['IAU' in _ for _ in matches.keys()]) here for the check and to get the appropriate match. I guess a bit of a question for @sgillies on what feels right to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually looking again at what this function does, why does rasterio attempt to check the authority at all?

Why not do something more compact like:

matches = self._matches(confidence_threshold=confidence_threshold)
if len(matches) < 1:
    return None
authority, code = next(iter(matches.items()))
return authority, code[0]

if walrus is supported in cython:

if not (matches := self._matches(confidence_threshold=confidence_threshold)):
    return None
authority, code = next(iter(matches.items()))
return authority, code[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh okay so I am reading the comment in the code right above more clearly now and the desire was that matches from other authorities could be found but prior to 1.2.7 EPSG was the only one considered, so the if statements were aimed to return EPSG first even if other codes had better matching scores.

On one hand this is probably fine but requires knowledge of all the authorities ahead of time and updating this function any time a new authority is desired to be supported. Not that new authorities get added to often but that is a bit annoying to have to deal with.

Alternatively, as there have been a few major version changes, it could make sense to change the behavior and now accept that whatever authority returns the highest score is probably the correct authority, and if EPSG is desired users can just use .to_epsg or call the private _matches function themselves. We could also make a public authority_matches function to empower users this way.

I just started looking at the OGR api now to see if there is an equivalent to pyproj.database.get_authorities, but haven't found it yet, but also think that a list of authority priorites would need to then be maintained in rasterio like the if statements so that doesn't really change anything for the better.

Thoughts? @vincentsarago @sgillies @snowman2 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just started looking at the OGR api now to see if there is an equivalent to pyproj.database.get_authorities, but haven't found it yet,

==> OSGeo/gdal#11130

Copy link
Member

Choose a reason for hiding this comment

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

Looks like I am late to the conversation here, but thought this information could be helpful with the design.

For pyproj, to_authority has an optional auth_name argument to only return results from a specific authority. That could potentially be used here to allow users to select the authority they want it they prefer a specific authority. If it isn't provided, the best match is returned regardless of the authority.

For this function, it appears to not return a match at all if an authority that is not preferred is identified (I may be missing something because I am on my phone). If what I am seeing is the case, is there a reason not to return a match if it isn't in the prioritized authority list?

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.

I think we should use the official authority name.

@AndrewAnnex AndrewAnnex requested a review from sgillies November 4, 2024 18:38
@AndrewAnnex
Copy link
Contributor Author

I think we should use the official authority name.

@sgillies I believe the conversation above resolved this question.

@sgillies
Copy link
Member

sgillies commented Nov 6, 2024

@AndrewAnnex Great! Now we need the appropriate changes in the code (I have a suggestion that you can accept) and tests.

Copy link
Contributor Author

@AndrewAnnex AndrewAnnex left a comment

Choose a reason for hiding this comment

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

heh thought I was writing the commit message

@snowman2
Copy link
Member

snowman2 commented Nov 7, 2024

Potential alternative: #3239

@AndrewAnnex
Copy link
Contributor Author

after looking at #3239, I think there is probably more work to be done in that pr to implement the changes to _matches, but this PR could still be merged in as a quick-patch in the meantime. Plus the new tests from this PR will ensure things keep working, I don't anticipate that the results of #3239 would break any code.

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! Let's merge this and refactor later (discussion in #3239).

@sgillies sgillies merged commit 60b8432 into rasterio:main Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants