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

sphynx
Copy link
Contributor

@sphynx 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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``
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

src/PJ_lccm.c Outdated
#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";
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps scale should be mentioned too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@kbevers kbevers 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 adding this. I've inlined some suggestions for improvements to the code, tests and docs.

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 Show resolved Hide resolved
@rouault
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
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
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
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants