-
Notifications
You must be signed in to change notification settings - Fork 15
MGRS Conversion fix #72
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
Conversation
a) Index out of bounds error due to typo in convertMGRSToUPS; clearly the index should have been 1 instead of 12. b) Conversions from geodetic to MGRS would fail for low southern latitudes (zone 0). Geotrans 3.7 has this problem fixed. The WorldWind conversion code was derived from NGA's Geotrans. Test case: Lat: -89.345400 deg, Lon: -48.930600 deg ==> MGRS: AZN 45208 47747
Made some formatting fixes as well as fixing a spelling mistake in one of the comments.
Moved the UTM_MGRS_test class to the correct location. It should be in the test folder so that it gets executed as part of the unit tests.
- Split up some of the methods into smaller self-contained methods. - Renamed some of the variables for clarity. - Changed the formatting by replacing tabs with spaces. Made some other formatting changes so that the code fits in with rest of code-base. - Added license-header at the top.
@gbburkhardt, I made some small fixes. The |
Hello, guys! Could you please do not forgot to make the same changes in my Android port when you finished. I do not want to lost convertor code-base synchronization between Java and Android. It will be harder for me to dive deep in to current issue context then you. Thanks. |
@Sufaev, I can help with the port of these fixes to the Android code-base. |
@wcmatthysen I have unreleased PR 16 in Android repository, which contain ported coordinate converters and graticules from Java. It will be perfect to add commit with all changes related to latest MGRS fixes to the same PR. By the way, I do not hear @emxsys for a long time. That's why PRs are still unreleased. Is he ok? |
@Sufaev, I'll have a look at that pull-request once we are done merging this one. I don't know. I haven't heard from him in a while. |
Well, as you can see, I still don't like the verbose coding style w.r.t. curly brackets. 😉 |
@wcmatthysen : I notice that you added a NASA copyright to the MGRS unit test file. Is that appropriate? NASA didn't write that code. Can a copyright be assigned to an entity without its knowledge or permission? I'm not worried, just curious. |
@gbburkhardt, I also don't like the curly braces on a separate line when it comes to styling. I think it is just important to keep the coding-style consistent with respect to the rest of the code-base. It might be something that we can discus in future depending on what happens to the upstream repository. If they suddenly start to contribute changes again then we might have to stick with the coding-style as it will be easier to pull in changes from them. I think we need to add the copyright at the top of each new file as the entire code-base is licensed under the NASA open-source license agreement. It doesn't matter if a NASA developer wrote the code or not. As contributors we agree to adhere to the license when we make contributions. |
Just teasing about the coding style. I strongly agree that only one
style should be used, otherwise the code gets really unreadable. I'll
try to be a better boy in the future....
…On 8/14/2019 9:20 AM, Wiehann Matthysen wrote:
@gbburkhardt <https://github.com/gbburkhardt>, I also don't like the
curly braces on a separate line when it comes to styling. I think it
is just important to keep the coding-style consistent with respect to
the rest of the code-base. It might be something that we can discus in
future depending on what happens to the upstream repository. If they
suddenly start to contribute changes again then we might have to stick
with the coding-style as it will be easier to pull in changes from them.
I think we need to add the copyright at the top of each new file as
the entire code-base is licensed under the NASA open-source license
agreement. It doesn't matter if a NASA developer wrote the code or
not. As contributors we agree to adhere to the license when we make
contributions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#72?email_source=notifications&email_token=ABI6HFF4ZWG2JKBO4HKJXV3QEQBBBA5CNFSM4ILQ6LJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4IYPXY#issuecomment-521242591>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI6HFFGUHMTZNJJSY4MJ3TQEQBBBANCNFSM4ILQ6LJA>.
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
|
@gbburkhardt, no worries. I am more worried about changing the coding-style and then the upstream repository suddenly becomes active again (though that looks doubtful at this point). If the upstream developers add changes and we need to pull those changes through to our repository here then we'll have a hard-time sorting through the changes if we have a lot of changes on our side related to code formatting. So, at this point, keeping the coding style as-is is more a case of us keeping as much in sync with the upstream repository as possible. Maybe down the line if we see that the upstream repository is truly dead we can consider switching to a more compact coding style. We'll have to wait and see what happens. |
Description of the Change
Fix two problems:
a) Index out of b typo in convertMGRSToUPS; clearly
the index should have been 1 instead of 12.ounds error due to
b) Conversions from geodetic to MGRS would fail for low southern
latitudes (zone 0). Geotrans 3.7 has this problem fixed. The WorldWind
conversion code was derived from NGA's Geotrans. Test case:
Lat: -89.345400 deg, Lon: -48.930600 deg ==> MGRS: AZN 45208 47747
Why Should This Be In Core?
Correct conversion from geodetic to MGRS
Benefits
Potential Drawbacks
None
Applicable Issues
Issue #67.