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

Error in Bertin1953? #1579

Closed
rschmunk opened this issue Sep 3, 2019 · 7 comments
Labels
bug

Comments

@rschmunk
Copy link
Contributor

@rschmunk rschmunk commented Sep 3, 2019

The final "north warp" at lines 69-71 in the Bertin1953 projection code has that

    if (xy.y > 0.) {
      xy.x *= 1. + d / 1.5 * xy.x * xy.x;
    }

However, in attempting to duplicate the rendering of the Bertin 1953 implemented in D3 and shown in the gallery at https://github.com/d3/d3-geo-projection , I found that the northern hemisphere wasn't coming out right. Double-checking the code for that warp, I found that the Javascript code on https://github.com/d3/d3-geo-projection/blob/master/src/bertin.js alters the value for xy.y (or r[1] in the d3-geo repo) rather than xy.x. Thus, if the JS code in the d3-geo repo is correct, then the code here in PROJ should read

    if (xy.y > 0.) {
      xy.y *= 1. + d / 1.5 * xy.x * xy.x;
    }

@Fil, can you confirm?

@Fil

This comment has been minimized.

Copy link
Contributor

@Fil Fil commented Sep 3, 2019

Looks like you're right — how did I not see that mistake? 😱6b0567b#diff-31a3d34b36547f14e56143393932f3efR71

Here's a drawing that shows what the correct version should be:

correct

The following shows what you would get with the incorrect code:
incorrect

(I think my problem when I ported the JS code to CPP is that I am struggling to draw with PROJ.)

How can we fix — and test — this?

@kbevers

This comment has been minimized.

Copy link
Member

@kbevers kbevers commented Sep 3, 2019

How can we fix — and test — this?

Seems like you've already made fix. Submit it in a pull request. As part of the pull request it would be great to have the figure in the docs updated also, so that the projection is displayed correctedly. Here is the current version: https://proj.org/operations/projections/bertin1953.html

You can update the figures by running the plot.py script in docs/source/plot. Something like python3 plot.py plotdefs.json ../source/operations/projections/images/ bertin1953

There's a conda environment file in the plot folder which can be used to set up a suitable environment for building the plots. It is a bit old so some packages may need to be updated but it shouldn't be too difficult to figure out.

Making the plot is a fairly good sanity test of the projection. Ideally we would also want some test coordinates in a gie file, for instance in test\gie\more_builtins.gie. Choose coordinates from the affected area so you can make sure that it performs as expected. Ideally you would find test coordinates in the paper that originally proposed the projection, but if I remember correctly from when this was introduced that was not possible. The next best thing is test material based on the D3 implementation.

@kbevers kbevers added the bug label Sep 3, 2019
@rschmunk

This comment has been minimized.

Copy link
Contributor Author

@rschmunk rschmunk commented Sep 3, 2019

I submitted PR #1582 to take care of the code issue. Someone else will have to handle updating any figure in the docs, etc. I only ran into this while trying to Implement the Bertin 1953 in Java. I found the PROJ implementation easier to read than the d3-geo version, but eventually realized that there must be a bug in the PROJ code.

@Fil

This comment has been minimized.

Copy link
Contributor

@Fil Fil commented Sep 3, 2019

Thanks! I've tried to follow @kbevers' procedure, but I'm stuck at compilation.

@kbevers

This comment has been minimized.

Copy link
Member

@kbevers kbevers commented Sep 16, 2019

Here's a drawing that shows what the correct version should be:
correct

@Fil Can I use the image above in the PROJ docs? The script I normally use to generate projection examples with does not cope with this projection very well.

@Fil

This comment has been minimized.

Copy link
Contributor

@Fil Fil commented Sep 16, 2019

@kbevers

This comment has been minimized.

Copy link
Member

@kbevers kbevers commented Sep 16, 2019

Yes, of course! Thank you

Thanks :-)

backporting bot pushed a commit that referenced this issue Sep 16, 2019
Northern hemisphere "warp" implemented incorrectly. See #1579
@kbevers kbevers closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.