Skip to content

Conversion from DMS to Decimal Degrees fails between 1 Degree and -1 Degree #25

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

Closed
jmmain69 opened this issue Nov 10, 2016 · 11 comments
Closed
Labels
Milestone

Comments

@jmmain69
Copy link

jmmain69 commented Nov 10, 2016

This issue occurs in gov.nasa.worldwind.geom.Angle fromDMS() and fromDMdS()

The formula for converting from Degrees Minutes Seconds to Decimal Degrees uses Math.signum(degrees) to provide a positive or negative number. This causes any value less than 1 degree to become 0.0, for example 00 degrees 59 minutes 59 seconds North would become 0.0 degrees in decimal degrees as signum(0) will return 0.

Math.signum(degrees) * (Math.abs(degrees) + minutes / 60d + seconds / 3600d)

@zglueck zglueck added the bug label Nov 10, 2016
@zglueck
Copy link
Contributor

zglueck commented Nov 10, 2016

@jmmain69 thanks for spotting this! I'll look into it.

@zglueck zglueck added this to the v2.1.0 milestone Nov 15, 2016
zglueck pushed a commit that referenced this issue Nov 16, 2016
… to an Angle using the fromDMS and fromDMdS functions. Prioritize compass directions when a sign and compass direction is provided to the fromDMS(String dmsString) function. Add unit tests verifying changes. Addresses Issue #25 which zeroed inputs when the degree value was zero.
@zglueck
Copy link
Contributor

zglueck commented Nov 16, 2016

@jmmain69 you were right, the signum function used was blanking values between -1 and 1.

As the team reviewed the issue, we noticed the documentation for fromDMS stated the degrees parameter should be positive. Despite noting this in method documentation, the original code was attempting to account for negative degree values. Bounds for the minutes and seconds were checked, but not the degrees, and as you pointed out, the signum function worked for all cases between, except between -1 and 1.

The intent and actual execution of the method were confusing, and while this works for a large chunk of values, the case you identified is a major issue.

We decided the best coarse of action is to enforce the documented intent of all values being positive for this function (and for fromDMdS), thereby requiring the application to compensate for negative angles. The signum functionality has been removed from the fromDMS and fromDMdS functions and an IllegalArgumentException will now be thrown for degree values less than zero. The function now is valid for the entire positive domain.

We did not take this step lightly, and understand, some applications may now experience an IllegalArgumentException on what had been previously working inputs (e.g. -45 degrees 5 minutes 10 seconds). This change removes ambiguity for negative inputs by requiring the application to determine the domain. We felt this was the best path forward in order to ensure the right values are provided from the function.

The Angle class does offer a fromDMS(String dmsString) function which will account for DMS angle values with a sign or compass direction. I encourage you and others to review its documentation and functionality as additional option.

Please feel free to continue the discussion here or at the forum, and thanks again for bring this to our attention.

@wcmatthysen
Copy link

wcmatthysen commented Nov 18, 2016

A simple fix for this, without breaking existing code is to have a special clause for 0-degrees, as in:

float sign = (degrees == 0) ? 1.0f : Math.signum(degrees);
return Angle.fromDegrees(sign * (Math.abs(degrees) + minutes / 60d + seconds / 3600d));

I think it would be still useful to allow negative degrees for this method, especially when constructing a LatLon from an Angle-pair. Reverting to the string-based parsing method is inefficient.

@zglueck
Copy link
Contributor

zglueck commented Nov 18, 2016

I think that would satisfy the 0 to 1 degree domain, but what about -1 to 0? Maybe I'm missing something in your approach, but if I wanted to represent 0 degrees 15 minutes 34 seconds west, it would now be 0 degrees 15 minutes 34 seconds east. Basically, -1 to 0 is complicated by the fact we can't have a -0 degree.

@wcmatthysen
Copy link

wcmatthysen commented Nov 18, 2016

Damn, I see. Couldn't we use a float for the degrees? We could detect the difference between positive and negative zero (see: this link). It just seems a bit harsh to suddenly change the behaviour of this method to disallow negative degree parameters, especially if there are code out there that depends on this behaviour.

@zglueck
Copy link
Contributor

zglueck commented Nov 18, 2016

Yeah the team really didn't want to break the existing functionality and the double negative zero idea did get discussed, but it really came down to this: As it was, there was a silent calculation failure that just returned bad results for a certain domain and the documentation did say positive (despite the efforts of the underlying code).

By leaving the sign determination up to the application, we remove any ambiguity presented by the API, and now, with the bounds being checked and exception being thrown, at least alert users. While looking into the issue I actually found a number of online javascript calculators with the same issue between -1 and 0. It really didn't feel good to limit the function, but it feels better than having a silent erroneous result being returned.

@wcmatthysen
Copy link

wcmatthysen commented Nov 18, 2016

Ok, I understand. However, wouldn't it then be feasible to introduce an additional fromDMS() method with a sign for the degrees as parameter? Then, at the very least we can fall back to a new method if we relied on the fromDMS() method to create LatLon objects.

@wcmatthysen
Copy link

I'm talking about something along the lines of:

public static Angle fromDMS(int sign, int degrees, int minutes, int seconds) { ... }

with sign being -1 or 1 (or maybe some other approach where sign is a boolean toggle or something).

@zglueck
Copy link
Contributor

zglueck commented Nov 28, 2016

We did discuss adding a function or modifying the existing signature. In the end, we decided incorporating the sign handle into a function signature wouldn't be clear. Leaving it up to the application is the safest and clearest approach.

@wcmatthysen
Copy link

I suppose the workaround could be Angle.fromDMS(deg, min, sec).multiply(-1) then.

@pdavidc
Copy link
Contributor

pdavidc commented Nov 28, 2016

That's exactly what we had in mind. Our thinking was: why complicate the method signature when the application-based solution is simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants