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 support for deg, rad and grad in unitconvert (fixes #1052), and document that it supports numeric factors (refs #1053) #1054

Merged
merged 2 commits into from Jun 22, 2018

Conversation

Projects
None yet
2 participants
@rouault
Member

rouault commented Jun 21, 2018

No description provided.

@kbevers

What happens if I do something like +proj=unitconvert +xy_in=us-ft +xy_out=grad or +proj=unitconvert +z_in=m +z_out=deg?

Apart from that consideration I only have a few minor details to correct.

###############################################################################

In the table below all angular units supported by PROJ `unitconvert` are listed.
(since PROJ 5.2.0)

This comment has been minimized.

@kbevers

kbevers Jun 21, 2018

Member

Please use .. versionadded:: 5.2.0 instead. Place it below the section header. That's consistent with the rest of the docs.

This comment has been minimized.

@rouault
units.
Horizontal input units. See :ref:`distance_units` and :ref:`angular_units`
for a list of available units. `<conversion_factor>` is the conversion factor
from the input unit to the International System unit.

This comment has been minimized.

@kbevers

kbevers Jun 21, 2018

Member

Perhaps just say that it is the conversion factor to meters? Otherwise I would say that "SI unit" is closer to standard nomenclature. Also valid below.

This comment has been minimized.

@rouault

rouault Jun 21, 2018

Member

"conversion factor to meters"... if it is linear unit, otherwise it is to radian. I can add that precision

This comment has been minimized.

@rouault


/***********************************************************************/
static double getUnitConversionFactor(const char* name,

This comment has been minimized.

@kbevers

kbevers Jun 21, 2018

Member

Good idea to add this function. Makes the code a bit easier to read. Small style nitpick: The rest of the functions use snake case so it looks a bit odd that this is named in camel case.

This comment has been minimized.

@rouault
@rouault

This comment has been minimized.

Member

rouault commented Jun 21, 2018

What happens if I do something like +proj=unitconvert +xy_in=us-ft +xy_out=grad or +proj=unitconvert +z_in=m +z_out=deg

Non-sense, but should we try to protect against this ?

@rouault rouault force-pushed the rouault:add_angular_units_to_unitconvert branch from e7626c0 to 7149043 Jun 21, 2018

@kbevers

This comment has been minimized.

Member

kbevers commented Jun 21, 2018

Non-sense, but should we try to protect against this ?

I didn't think it would be necessary to protect against creation of non-sense pipelines but of course there were complaints that you could do just that. This is somewhat similar so maybe it is better to just take care of that in advance. It is not a top priority for me - it can also be handled by explaining the behaviour in the docs.


In the table below all angular units supported by PROJ `unitconvert` are listed.

.. versionadded:: 5.2.0

This comment has been minimized.

@kbevers

kbevers Jun 21, 2018

Member

If you put it right below the heading it is more clear that it is the whole section that was added in version 5.2.0

This comment has been minimized.

@rouault

rouault Jun 21, 2018

Member

fixed

@rouault rouault force-pushed the rouault:add_angular_units_to_unitconvert branch from 79e6904 to d40ec8d Jun 21, 2018

@rouault

This comment has been minimized.

Member

rouault commented Jun 21, 2018

ok, I've added a consistency check for the unit types in xy_in vs xy_out and z_in vs z_out

@@ -583,6 +583,7 @@ enum deprecated_constants_for_now_dropped_analytical_factors {
#define PJD_ERR_ELLIPSOIDAL_UNSUPPORTED -56
#define PJD_ERR_TOO_MANY_INITS -57
#define PJD_ERR_INVALID_ARG -58
#define PJD_ERR_INCONSISTENT_UNIT -59

This comment has been minimized.

@kbevers

kbevers Jun 21, 2018

Member

You should add a string representation in src/pj_strerrno.c as well

This comment has been minimized.

@rouault

rouault Jun 21, 2018

Member

Good point. I've added a comment at the end of the enumeration to update pj_strerrno.c AND pj_transform.c as well (and I've sanitized the access to the transient_error array in it)

@kbevers

This comment has been minimized.

Member

kbevers commented Jun 21, 2018

Add the string representation of the new error code and I am happy with this. It's a nice new feature to have!

@kbevers kbevers added this to the 5.2.0 milestone Jun 21, 2018

rouault added some commits Jun 21, 2018

Add support for deg, rad and grad in unitconvert (fixes #1052), and d…
…ocument that it supports numeric factors (refs #1053)

@rouault rouault force-pushed the rouault:add_angular_units_to_unitconvert branch from d40ec8d to 5411bd3 Jun 21, 2018

@kbevers kbevers merged commit 722f22d into OSGeo:master Jun 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 77.687%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment