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

Feature/precision library #7

Open
wants to merge 5 commits into
base: feature/precision_library
Choose a base branch
from

Conversation

ionutab
Copy link

@ionutab ionutab commented Mar 13, 2021

Hello,
I've redone your translation of the initial algorithm using the Apfloat library.
There were some issues in your translation in the following methods:
cartesianToGeographic
geographicToLambertAlg003
latitudeISOFromLat
I've had to modify some reference test values because of minor differences caused by the precision algorithm.
Please let me know if there's any other test case you would like me to add.
Thank you.

@@ -45,15 +45,19 @@ public void Lambert93BugTest() {
LambertPoint pt = Lambert.convertToWGS84Deg(668832.5384, 6950138.7285, LambertZone.Lambert93);
assertEquals(2.56865, pt.getX().doubleValue(), 0.0001);
assertEquals(49.64961, pt.getY().doubleValue(), 0.0001);

}

@Test
public void LambertIIExtendedToWgs84Test() {

Choose a reason for hiding this comment

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

I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), Nm: Method names should start with a lower case letter (NM_METHOD_NAMING_CONVENTION).
Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

Copy link
Author

Choose a reason for hiding this comment

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

Hello
I left the test method names exactly like they were in the initial repo.
Yes, this is bad practice, but fixing this was not part of my reasons for this pull request.
Thank you.

Choose a reason for hiding this comment

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

noticed

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.

2 participants