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

Add Lambert Conic Conformal (2SP Michigan) projection #1142

Merged
merged 1 commit into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@sphynx
Copy link
Contributor

sphynx commented Oct 4, 2018

Fixes #940

@@ -0,0 +1,5 @@
.. option:: +scale=<value>

Ellipsoid scale factor. Determines ellipsoid scale factor used in the projection.

This comment has been minimized.

@rouault

rouault Oct 4, 2018

Member

As this is a very specific options to this projection method, I would keep it inline in lccm.rst

This comment has been minimized.

@sphynx

sphynx Oct 4, 2018

Author Contributor

Yes, it makes sense, will change that.

I was also not sure about the name 'scale'. I think 'k_0' (and 'k' in the past) is generally used for scaling, but since this is a different kind of scaling I decided that it would be better to have a separate option than to reuse 'k_0' somehow ('k_0' can still be used in conjunction with 'scale' though).

:align: center
:alt: Lambert Conformal Conic (2SP Michigan)

proj-string: ``+proj=lccm +lon_0=-90 +k=1.0000382``

This comment has been minimized.

@rouault

rouault Oct 4, 2018

Member

should be scale and not k ?
(I was wondering if 'k' shouldn't be used instead of 'scale', but its meaning in LCC_2SP Michigan is very specific, so perhaps scale is fine)

This comment has been minimized.

@sphynx

sphynx Oct 4, 2018

Author Contributor

Good catch! As I said in the previous comment I tried 'k' first, but then switched to 'scale', but forgot to change the doc.

#include "proj_math.h"

PROJ_HEAD(lccm, "Lambert Conformal Conic (2SP Michigan)")
"\n\tConic, Sph&Ell\n\tlat_1= and lat_2= or lat_0";

This comment has been minimized.

@rouault

rouault Oct 4, 2018

Member

Perhaps scale should be mentioned too

This comment has been minimized.

@sphynx

sphynx Oct 4, 2018

Author Contributor

Ok, should I just add it to the end? i.e. "lat_1= and lat_2= or lat_0, scale="?

@kbevers
Copy link
Member

kbevers left a comment

Thanks for adding this. I've inlined some suggestions for improvements to the code, tests and docs.

Show resolved Hide resolved src/PJ_lccm.c Outdated
Show resolved Hide resolved src/PJ_lccm.c Outdated
Show resolved Hide resolved src/PJ_lccm.c Outdated
Show resolved Hide resolved test/gie/builtins.gie Outdated
Show resolved Hide resolved test/gie/more_builtins.gie
Show resolved Hide resolved docs/source/operations/projections/lccm.rst Outdated
Show resolved Hide resolved docs/source/operations/projections/lccm.rst Outdated
Show resolved Hide resolved src/PJ_lcc.c Outdated
@rouault

This comment has been minimized.

Copy link
Member

rouault commented Oct 10, 2018

Looks good to me! Would you mind squashing all the commits into a single one ? (I can also do it with the github UI but then the commit would appear as having you and me as authors)

@sphynx sphynx force-pushed the sphynx:proj-lcc-2sp-michigan branch from 08f0548 to 8acb481 Oct 10, 2018

@sphynx sphynx force-pushed the sphynx:proj-lcc-2sp-michigan branch from 8acb481 to 8fc1197 Oct 10, 2018

@sphynx

This comment has been minimized.

Copy link
Contributor Author

sphynx commented Oct 10, 2018

Looks good to me! Would you mind squashing all the commits into a single one ? (I can also do it with the github UI but then the commit would appear as having you and me as authors)

Squashed and pushed with --force.

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Oct 11, 2018

It also looks good to me. Thanks for putting up with our annoying requests. I think this has turned out to be a great PR giving us both better documentation, tests and understanding of the projection. Good job!

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

2 checks passed

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

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

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.