Skip to content

ROT accuracy issue #1909

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
dinsmoro opened this issue Nov 9, 2024 · 6 comments
Closed

ROT accuracy issue #1909

dinsmoro opened this issue Nov 9, 2024 · 6 comments
Labels
solved WIP Work In Progress: PR acceptance will be considered when this label is removed.

Comments

@dinsmoro
Copy link

dinsmoro commented Nov 9, 2024

I noticed that ROT doesn't give the right outputs:

GDL> a=findgen(3,3)
GDL> print, a
      0.00000      1.00000      2.00000
      3.00000      4.00000      5.00000
      6.00000      7.00000      8.00000
GDL> print, rot(a,180.)
      5.00000      7.00000      6.00000
      1.00000      0.00000      3.00000
      1.00000      0.00000      0.00000

I tried some other older ROT functions from NASA personnel, but they performed the same which I suspect points to the poly_2d call (per we use the c++ internal POLY_2D code to do the job comment). I can't find that call in the repo, though.

This issue has popped up before, maybe system-dependent?:
https://cow.physics.wisc.edu/~craigm/idl/archive/msg03150.html

And IDL on a separate computer does give the expected values:

IDL> a=findgen(3,3)
IDL> print, a
      0.00000      1.00000      2.00000
      3.00000      4.00000      5.00000
      6.00000      7.00000      8.00000
IDL> print, rot(a,180.)
      8.00000      7.00000      6.00000
      5.00000      4.00000      3.00000
      2.00000      1.00000      7.34788e-16
@alaingdl
Copy link
Contributor

Thank you for this important bug report. I am sad that such an important bug was not detected before :(
And no test in the testsuite. Sorry.

@dinsmoro
Copy link
Author

Let me know if I can help - I'll take a stab at poly_2d if y'all need the help and you let me know where it is, but I can't guarantee anything since I don't know C++! But I can try!

@alaingdl
Copy link
Contributor

((I don't understand why we never realized that this code, which is big a complex, never gave problem before !))

OK, I found one problem in the math_fun_jmpg.cpp code, adding round() goes in the good direction.
I have another trouble within permutation of i & j index, TBC

          DLong px = (DLong) round(P[0] + P[1] * (DDouble) i + P[2] * (DDouble) i);
          DLong py = (DLong) round(Q[0] + Q[1] * (DDouble) j + Q[2] * (DDouble) j);

I also realized that we have no systematic tests related to that part of code (TBD)

@GillesDuvert
Copy link
Contributor

I've looked at it. There are indeed at lot of cases where the result is different, @dinsmoro 's one is only the tip of the iceberg.
I've found it mostly comes from all the accumulated roudings. Will prepare a PR soon, adding I hope some speed too, as I've noted that poly_2d is very slow.
The formula with the i and j is ok.

@GillesDuvert GillesDuvert added bug Something isn't working priority! labels Nov 16, 2024
@GillesDuvert
Copy link
Contributor

BTW, IMHO there is no fault in the current code. It just does not behave as IDL does, but only because there was a different choice (or absence of) on the float-to-integer truncation when it comes to tell which pixel is whom. But we have to reproduce the IDL results.

GillesDuvert added a commit to GillesDuvert/gdl that referenced this issue Nov 26, 2024
…language#1909 (Use Floats for coordinates) 3) much improved speed !

rewrite congrid to use poly_2d similarly to IDL. Removed all tentatives to 'do better' (with dimensions).
separate poly_2d from math_fun_jmg
add poly_2d basic test
@GillesDuvert
Copy link
Contributor

Will be solved as soon as #1919 is merged.

@GillesDuvert GillesDuvert added solved WIP Work In Progress: PR acceptance will be considered when this label is removed. and removed bug Something isn't working priority! labels Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved WIP Work In Progress: PR acceptance will be considered when this label is removed.
Projects
None yet
Development

No branches or pull requests

3 participants