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

Get geoid models for a CRS code #2681

Merged
merged 10 commits into from Apr 24, 2021
Merged

Conversation

jjimenezshaw
Copy link
Contributor

  • Tests added
  • Added clear title that can be used to generate release notes
  • Fully documented, including updating docs/source/*.rst for new API

Given a CRS code, this new method returns the list of possible geoid models that apply to it.
(use case: give the user a list of available geoid models, to select the desired one).

The sql query has two parts. The second covers CRSs like EPSG:6360, that is the ftUS version of EPSG:5703 (NAVD88 height).

In a later PR, I was thinking on adding at least these geoid models to the db:

  • AUSGeoid2020,AUSGeoid09, AUSGeoid98 for AHD height (Australia)
  • RAF09, RAF18 for NGF-IGN69 height (France)
    (those are the cases I found with more than one grid file with the same target VCRS)

CC @snowman2

test/unit/test_factory.cpp Outdated Show resolved Hide resolved
test/unit/test_factory.cpp Outdated Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Apr 22, 2021

this looks OK to me. I guess we need a C API for it to be usable by pyproj

@jjimenezshaw
Copy link
Contributor Author

this looks OK to me. I guess we need a C API for it to be usable by pyproj

Do you mean in this PR, or in a later one? I can do any.

@kbevers
Copy link
Member

kbevers commented Apr 22, 2021

Do you mean in this PR, or in a later one? I can do any.

Put it in this one, please

@rouault
Copy link
Member

rouault commented Apr 22, 2021

#2683 brings a new conversion method EPSG:1104 "Change Of Vertical Unit" that is a variant of EPSG:1069 bit without the conversion factor specified (it must be infered from the source and target CRS). Not sure how it impacts your PR but wanted to raise that as I see you explicitly select on 1069

@rouault
Copy link
Member

rouault commented Apr 23, 2021

Can you rebase that on top of latest master that has now the EPSG v10.019 update ?

@jjimenezshaw
Copy link
Contributor Author

Can you rebase that on top of latest master that has now the EPSG v10.019 update ?

I am working on that right now, also adding EPSG:1104 "Change Of Vertical Unit"
Thanks for the heads-up yesterday.

@jjimenezshaw
Copy link
Contributor Author

C API added.
I do not know if the function names follow any rule. I tried to make it similar to others. Do not hesitate to suggest any other name.

@jjimenezshaw
Copy link
Contributor Author

About clang-format, I have it embedded in my IDE (vscode)... and proj.h got totally modified by it.
Does it make sense to add // clang-format off and on to proj.h to prevent that kind of problem? (In case we do not want proj.h to be automatically formatted by clang-format)

@rouault
Copy link
Member

rouault commented Apr 23, 2021

About clang-format, I have it embedded in my IDE (vscode)... and proj.h got totally modified by it.

We have a scripts/reformat_cpp.sh script that applies it only a restricted set of files. Will not probably help you on Windows (or maybe if you run it from WSL with Ubuntu 20.04)

@jjimenezshaw
Copy link
Contributor Author

We have a scripts/reformat_cpp.sh script

I am developing in native Ubuntu 20.04. My vscode automatically applies clang-format when I save any file, what is very convenient (in general). If I have to edit proj.h, then the whole file is automatically formatted. I had to use vim (that is not a problem). I was asking for the clang-format guard to avoid other people to have the same issue, and maybe commit the file with all those modifications.

Anyway, it is not a big deal.

@rouault
Copy link
Member

rouault commented Apr 23, 2021

I was asking for the clang-format guard to avoid other people to have the same issue, and maybe commit the file with all those modifications.

There are tons of other files in the repo which aren't clang-format'ed. Maybe at some point we should apply a huge reformatting to all .h*/.c*. Or maybe there's a file that vscode could read which would indicate it which files to reformat and which not ?

@rouault rouault self-requested a review April 23, 2021 20:32
@rouault rouault added this to the 8.1.0 milestone Apr 24, 2021
@rouault rouault merged commit 93dc842 into OSGeo:master Apr 24, 2021
@jjimenezshaw
Copy link
Contributor Author

Maybe at some point we should apply a huge reformatting to all .h*/.c*.

I would say so.
At some point with low development activity, I would say to apply it everywhere. If there are specific files that (for any reason) we do not want to format, just add the guard comment.
Maybe also a github "action" that checks that the format is right too.
The main inconvenience could be the backport of bugs. To minimize the impact, we should do it a little bit before a release.

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.

None yet

3 participants