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

S2 projection #2749

Merged
merged 15 commits into from
Jul 13, 2021
Merged

S2 projection #2749

merged 15 commits into from
Jul 13, 2021

Conversation

marcus-elia
Copy link
Contributor

I added the S2 family of projections to PROJ. The structure of s2.cpp is based on the qsc.cpp file, and the math functions are from S2 (s2coords.h and vector.h).

The face can be specified similarly to the QSC projection, using +lat_0 and +lon_0.

For S2, there are three choices for how to convert from "Cube-space" (u,v) to "Cell-space" (s,t) coordinates (see the S2 Cells page). You can specify +UTtoST to be an element of {linear, quadratic, tangent, none} to control which function is used to convert from u,v to s,t coordinates (see the comments in the S2 code for a description of them). It defaults to quadratic if +UVtoST is not given, the default mode in the S2 code.

Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.
In addition to addressing the remarks I made inline, we'd also need tests in test/gie/builtins.gie and a documentation page in docs/source/operations/projections

include/proj/coordinateoperation.hpp Outdated Show resolved Hide resolved
src/fwd.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/proj.h Outdated
* Purpose: Revised, experimental API for PROJ.4, intended as the foundation
* for added geodetic functionality.
*
/******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

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

this change should be reverted

src/proj_internal.h Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Show resolved Hide resolved
docs/source/operations/projections/s2.rst Show resolved Hide resolved
docs/source/operations/projections/s2.rst Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Show resolved Hide resolved
test/gie/builtins.gie Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Jun 21, 2021

@kbevers This projection method forces the unit sphere. This is a bit at odds with other projection methods, but might be idiomatic to this method. Should we accept it as such ? Another alternative would be to make it honor the semi-major axis, but people could use "+ellps=WGS84 +a=1" to force to the unit ellipsoid.

@kbevers
Copy link
Member

kbevers commented Jun 22, 2021

This is a bit at odds with other projection methods, but might be idiomatic to this method. Should we accept it as such ?

I don't know enough about this projection and it's potential use-cases to make an informed decision. If I understand correctly the output coordinates are always limited to be between 0 and 1. I wonder if that is the actual end product stored in databases etc or if they are scaled after the fact.

@elfprince13
Copy link

elfprince13 commented Jun 22, 2021 via email

@rouault rouault added this to the 8.2.0 milestone Jun 28, 2021
src/projections/s2.cpp Outdated Show resolved Hide resolved
src/projections/s2.cpp Outdated Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Jun 29, 2021

Some of the tests fail on a Linux i386 32bit build. See https://github.com/OSGeo/PROJ/pull/2749/checks?check_run_id=2942734733 . Not sure if the accuracy of the computation (probably an excess of accuracy due to 80 bit double) can be fixed or if the tolerance on the expected output should be relaxed.

@marcus-elia
Copy link
Contributor Author

@rouault I am interesting running the unit tests on that failing 32-bit build using a sphere instead of an ellipsoid, in case the precision issues come from the ellipsoid -> sphere conversion. Is there a way to modify and run the tests without committing/pushing?

@rouault
Copy link
Member

rouault commented Jul 6, 2021

Is there a way to modify and run the tests without committing/pushing?

you can do similarly as the CI build:
start a Docker image from the root of the PROJ repo:
docker run -it -e CI -e WORK_DIR="$PWD" -v $PWD:$PWD ubuntu:20.04
inside it:
$WORK_DIR/.github/workflows/linux_gcc_32bit/start.sh
and then you should be able to run / debug

@marcus-elia
Copy link
Contributor Author

I'm getting an error on
$WORK_DIR/.github/workflows/linux_gcc_32bit/start.sh
, and I've attached the output:
output.log
Can you help me figure out what is going wrong?

@rouault
Copy link
Member

rouault commented Jul 8, 2021


Can you help me figure out what is going wrong?

You probably need to clean your git checkout as there's probably conflicts with already built objects.
I generally do such debugging sessions in a dedicated git worktree : git worktree add ../debug_linux_32bit , cd into it, and then start docker from there

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

4 participants