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

the Bertin 1953 projection #1133

Merged
merged 5 commits into from Oct 11, 2018

Conversation

Projects
None yet
2 participants
@Fil
Copy link
Contributor

Fil commented Sep 21, 2018

closes #1120

@Fil Fil force-pushed the Fil:bertin1953 branch from e023a3c to fce77eb Sep 21, 2018

@Fil Fil force-pushed the Fil:bertin1953 branch from fce77eb to c996298 Sep 21, 2018

@kbevers
Copy link
Member

kbevers left a comment

Thanks for opening this PR.

I have added some inline comments which I think will improve the quality of this PR. In addition I a few general comments/changes I'd like to see before this gets merged:

  1. We need tests.
  2. I'd prefer if the code didn't use camelCase variable names. Generally speaking (there are exceptions), PROJ uses snake_case.

I would like to see a more formal mathematical description of this projection. As I understand it, the projection is basically the Hammer projected subjected to a rotation and some inital warping. I'd be interested to see if we couldn't to the same thing by constructing a pipeline of some more basic operations. This is just me thinking aloud, not something that should be pursued in this PR :-)

Show resolved Hide resolved src/PJ_bertin1953.c Outdated
Show resolved Hide resolved src/PJ_bertin1953.c Outdated
Show resolved Hide resolved src/PJ_bertin1953.c Outdated
Show resolved Hide resolved src/PJ_bertin1953.c Outdated
Show resolved Hide resolved src/PJ_bertin1953.c Outdated
Further reading
################################################################################

#. Philippe Rivière (2017). `Bertin Projection (1953) <https://visionscarto.net/bertin-projection-1953>`, Visionscarto.net.

This comment has been minimized.

@kbevers

kbevers Sep 24, 2018

Member

Is this the best reference we have? If possible, I'd like to have a reference to the mathematical description as well, preferably an academic paper.

This comment has been minimized.

@Fil

Fil Sep 24, 2018

Author Contributor

Well if you read that modest blog post, you'll know :)
(I wish I had time and resources to publish this as an academic paper.)

This comment has been minimized.

@kbevers

kbevers Sep 24, 2018

Member

I did, but it wasn't clear to me if something else was published.

You are welcome to use the PROJ docs as a platform for describing the mathematics behind the projection.

This comment has been minimized.

@Fil

Fil Sep 24, 2018

Author Contributor

PS: I agree that my construction is totally ad-hoc, and that another formula might be much nicer mathematically (esp. with second and third derivatives). It's the best I could come up with as a first approximation, and I'm very open to making an upgraded formula at some point (provided it still respects the original drawing).

Show resolved Hide resolved docs/source/operations/projections/bertin1953.rst Outdated
phi -= 0.8 * d * sin(phi + M_PI / 2.);
}

/* Project with Hammer (1.68,2) */

This comment has been minimized.

@kbevers

kbevers Sep 24, 2018

Member

So this is the same Hammer projection as defined in PJ_hammer.c, yes? Why not use that directly by instantiating a Hammer specific PJ object with Q->P_hammer = proj_create(P->ctx, '+proj=hammer ...') in the projection setup and call pj_fwd(Q->P_hammer, lp) here?

This avoids duplicate code and potential bugs in Hammer will be fixed both places if necessary.

This comment has been minimized.

@Fil

Fil Sep 24, 2018

Author Contributor

Yes I wanted to do that, but couldn't find an example in the src/

This comment has been minimized.

@kbevers

kbevers Sep 24, 2018

Member

Something similar is done in PJ_deformationc.:

proj.4/src/PJ_deformation.c

Lines 270 to 272 in 0b32b84

Q->cart = proj_create(P->ctx, "+proj=cart");
if (Q->cart == 0)
return destructor(P, ENOMEM);

geodetic.lpz = pj_inv3d(cartesian, P->opaque->cart);

You'll have to adjust it a bit but the general idea is there. Also notice that you need to create a custom descructor for the PJ object. There's an example for that in the same file as well.

This comment has been minimized.

@Fil

Fil Sep 24, 2018

Author Contributor

Feel free to add this! It's my first contribution to Proj.4 and it had been a looong time since I coded anything in C :-)

Show resolved Hide resolved docs/source/operations/projections/bertin1953.rst Outdated

Fil and others added some commits Sep 24, 2018

Req. changes for Bertin1953:
- classification
- tests
- coding style

@kbevers kbevers added this to the 6.0.0 milestone Oct 11, 2018

@kbevers kbevers merged commit 249588a into OSGeo:master Oct 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.1%) to 78.077%
Details

@Fil Fil deleted the Fil:bertin1953 branch Oct 12, 2018

@Fil

This comment has been minimized.

Copy link
Contributor Author

Fil commented Oct 12, 2018

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.