Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRS PROJ string parser: accept +ellps/+a/+rf/... compatible of +datum… #3171

Closed
wants to merge 1 commit into from

Conversation

rouault
Copy link
Member

@rouault rouault commented Apr 20, 2022

…, but error out if there are not (fixes #3170)

Probably not appropriate for backport

@rhuijben
Copy link
Contributor

rhuijben commented May 2, 2022

If we can't get this in 9.x, can we at least get some of this in the logging or warning output from parsing, like from wkt.

I'd rather see a dependable behavior then silent ignores. So you would have my +1 for merging, but I also see the backwards compatibility story...

@rouault
Copy link
Member Author

rouault commented May 2, 2022

@kbevers your take on the backport story of this in a stable branch ? I'm rather not confident. I wouldn't be surprised at all that there are odd cases of altering an ellipsoid +ellps with some +a/+rf/etc that haven't been identified. During development, I had to tweak the code to make a couple odd tests pass

@busstoptaktik
Copy link
Member

I'm -0 on this, and somewhat concerned. Not with respect to the problem at hand (#3170), but more in the general case of fear of painting ourselves deeper into some of the most unfortunate corners of current and past versions of the OGC/ISO geospatial model which, at least historically, have been at odds with the geodetic realities, focusing as they do (or at least did) on (non-existing) definitions of geodetic datums, rather than (stochastically derivable) relations between them.

+datum=... provides information about the actual datum in use, not just about its associated ellipsoid. So while equivalencing +datum=... and +ellps=... may not make any numerical difference for the problem at hand, it makes a huge difference with respect to user education, and limits the gamut of potential forward evolutionary paths for PROJ.

As the requirement for coordinate accuracy grows, so does the necessity of actually following the stochastic geodetic realities, rather than the more deterministic everyday-experience-compatible pseudo-geodetic mental models. When these are at odds, having a specified datum on both the source and the target side of the scene, at least makes it possible to check that a transformation explicitly selected by the user, actually transforms between the two systems at hand.

So blurring the difference between datums and ellipsoids is an efficient way of digging holes in the road toward the support of inevitable future use cases.

I can hardly claim to be against merging this PR, since it solves a concrete problem, but #3170 really sheds light on a more general problem that will bite us in the future: For higher accuracy, the API focus need to shift from definitions to transformations. Not just trying to guess what the user needs, but making it easier to ask for a specific transformation, then actually doing what the user asks for (i.e.,at the command line level, somehow making cs2csand cct confluence, and ditto at the API level).

This is one of the 4 reasons I started work on Rust Geodesy - not as a replacement for PROJ (not even remotely), but as an experimental platform for simplified data flows, and for developing concepts and potential solutions for predictable future problems. If these concepts and solutions make their way into PROJ in a few years, they may actually become useful for geodetic everyday work - but painting PROJ into too many corners in the meantime may impossibilize necessary innovations - from Rust Geodesy or elsewhere.

While sounding the alarm here, I do not actually think we're dealing with a huge problem, but I find it quite necessary to raise awareness that trying to provide deterministic solutions to stochastic problems will run us all out of steam sooner or later.

@kbevers
Copy link
Member

kbevers commented May 3, 2022

your take on the backport story of this in a stable branch ?

I don't think this should go into 9.0.1. It's a change in behaviour but not interface so I think it would be okay to put it in 9.1.0. I understand your worries though, we would be changing behaviour that someone might rely on at the moment. Maybe it would be smarter to just properly document the current behaviour? On the other hand we are in legacy PROJ.4-string territory that should be avoided and users can't expect this to function perfectly.

@rhuijben
Copy link
Contributor

The 9.1 ship sailed...

@kbevers
Copy link
Member

kbevers commented Aug 30, 2022

The 9.1 ship sailed...

Was this important to you? Two developers are questioning the approach here and the community hasn't been particularly vocal about needing this change. I'm okay with it not landing in 9.1.0 but happy to reconsider if this proves to be more needed than I realize.

@rhuijben
Copy link
Contributor

No it was not important, but we should somehow make a decision for the future on how we handle those 'error' cases. We can target it for 9.1 or postpone to 10... or decide not to do it at all..

Personally I would like to see which arguments are ignored via some kind of warning notification, assuming we can't make this an error. (Which I don't think we can. Adding errors to well defined transforms that always worked... Sounds like a breaking error)

@kbevers
Copy link
Member

kbevers commented Aug 31, 2022

Personally I would like to see which arguments are ignored via some kind of warning notification, assuming we can't make this an error

I would welcome a RFC on the topic. Especially if it's backed up by code. Handling of errors and warnings has always been difficult and it would be good to have a better defined strategy. Error-handling was refined a bit some time ago but warnings (and maybe deprecations?) could be definitely be handled better.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2023

The PROJ project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last two months and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add terminal output examples if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale label Jul 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 2 months. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the PROJ project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User isn't informed that information from proj4 string has been ignored in making CRS
4 participants