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

Fix #1074. Fix unit conversion factors for geod. #1075

Merged
merged 3 commits into from Jul 12, 2018
Merged

Conversation

@cffk
Copy link
Contributor

@cffk cffk commented Jul 11, 2018

Previously, unit conversion using atof(unit_list[i].to_meter) which
gives the wrong answer with, e.g., "1/10". Now it directly uses
unit_list[i].factor (e.g., 0.1). This fixes #1074.

Also fix all the conversion factors for the US Surveyor units so that
they are the closest doubles. E.g., the conversion factors for US
feet are

    factor              rel error
old 0.304800609601219   6e-16
    12/39.37            1e-16  
now 1200/3937.0         5e-17

Maybe someone should check the Indian units (but it's possible that
India and Pakistan have different standards).

Previously, unit conversion using atof(unit_list[i].to_meter) which
gives the wrong answer with, e.g., "1/10".  Now it directly uses
unit_list[i].factor (e.g., 0.1).

Also fix all the conversion factors for the US Surveyor units so that
they are the closest doubles.  E.g., the conversion factors for US
feet are

        factor              rel error
    old 0.304800609601219   6e-16
        12/39.37            1e-16  
    now 1200/3937.0         5e-17

Maybe someone should check the Indian units (but it's possible that
India and Pakistan have different standards).
@kbevers
Copy link
Member

@kbevers kbevers commented Jul 11, 2018

Thanks! I figured this would be a simple fix.

Changing the unit conversion factors seem to break some tests, though.

Loading

@cffk
Copy link
Contributor Author

@cffk cffk commented Jul 11, 2018

I know! We'll see... The US-inch was way off, 2 parts in a million. The others are down in the noise.

Loading

Bletch, pj_init also decodes the to_meters field of pj_unit, but only
handles plain numbers or 1/nnn, but not 1./nnn or mmm/nnn.  (So the
original code would have not decoded the us-in entry properly.)

Probably pj_init should be changed to use the factor field instead.

Alternatively pj_init should look for a "/" anywhere in the field and
decode the numerator and denominator as separate doubles.  This would
be needed if ratios are ever to be entered directly by the user (this
is the strategy used by GeographicLib).  And then, of course, the
functionality should be provided by a separate utility function.
@kbevers
Copy link
Member

@kbevers kbevers commented Jul 11, 2018

Probably pj_init should be changed to use the factor field instead.

Yes, that would be the logical progression. I added the factor field while introducing the unitconvert operation, so it is a fairly new addition. At the time I didn't think of using it in the rest of the code.

Loading

@cffk cffk merged commit 2fe6db2 into OSGeo:master Jul 12, 2018
4 checks passed
Loading
@kbevers kbevers added this to the 5.2.0 milestone Aug 20, 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
Linked issues

Successfully merging this pull request may close these issues.

2 participants