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

c: fix "a", revert to 2-iter exact #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Artoria2e5
Copy link
Contributor

Commits cd7a6a4 killed the 2-iter exact algo. This commit brings it back.

Commits cd7a6a4 killed the 2-iter exact algo. This commit brings it back.
Copy link
Contributor

@adclose adclose left a comment

Choose a reason for hiding this comment

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

Changes are all non-breaking. and should provide a significant improvement in performance.

c/transform.c Outdated
@@ -24,26 +24,26 @@ INLINE static int outOfChina(double lat, double lng) {
return 0;
}

#define EARTH_R 6378137.0
#define EARTH_R 6371000

void transform(double x, double y, double *lat, double *lng) {
double xy = x * y;
double absX = sqrt(fabs(x));
Copy link
Contributor

Choose a reason for hiding this comment

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

better name for this should be sqrtX not absX

c/transform.c Outdated
pLng = gcjLng + dLng;
double dLat, dLng;
// n_iter=2: centimeter precision, n_iter=5: double precision
const int n_iter = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

If your going to drop the threshold check I suggest 3 or 4 iterations not just 2 as my tests showed that to get to the right precision most times, I iterated 3 times.

for (i = 0; i < n_iter; i++) {
delta(*wgsLat, *wgsLng, &dLat, &dLng);
*wgsLat = gcjLat - dLat;
*wgsLng = gcjLng - dLng;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The I'm glad to see this method changed as it's so much faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note... It would be really nice to see consistency between versions and variable names. X_Pi,xPi,yPi,MPi.....
and consistent definitions across implementations mPi = 3000Pi/180; xPi = xmPi for example..
exact transform implemented via cajun method everywhere... or left as half distance method, and mars2wgs as cajin. Just so long as it's all consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The C impl is actually the first instance where a caijun-esque method is introduced to eviltransform. Like I said in the PR, it's ditched in an "optimize" commit, the only productive part of which is fixing a "fabs" declaraction.

Copy link
Contributor Author

@Artoria2e5 Artoria2e5 Jan 12, 2018

Choose a reason for hiding this comment

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

And yeah the whole varname thing is crazy. We probably don't even need all those, as many of them like x*pi and sqrt(fabs(x)) are really subexpression optimizations that can be done automatically by the compiler as long as you allow it to do associative math on floats.

Yeah, right, good call -- let me think really hard about putting some pragmas to tell the compilers…

Copy link
Contributor

Choose a reason for hiding this comment

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

I like defining subexpressions or constants that otherwise would be recalculated multiple times later , to limit the clock cycles needed for repetitive calculations. I just like to see consistent naming.

Quite frankly I would like to see these all dealt with in some -fassociative-math pragma
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