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

BUG: proj4 over option causes problems with polar projections #1772

Merged
merged 1 commit into from Jan 8, 2016

Conversation

Projects
None yet
3 participants
@danlipsa
Contributor

danlipsa commented Jan 7, 2016

On certain machines, +over option to proj4 results in wrong
projections. It is not clear why there is a difference between different
computers - we have seen this behavior on both unix and mac. +over
does not wrap points outside of -180, 180. This is not necessary for
polar projections so we remove this option in that case.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 7, 2016

@danlipsa THANKS! I will do a bit of testing for lambert as well, maybe it's better to go with a simlar approach than the one we have for labels, create a list of known projection that need over and only set it for these, that should limit the bug a bit further, no? see: https://github.com/UV-CDAT/uvcdat/blob/master/Packages/vcs/Lib/projection.py#L21

@danlipsa danlipsa force-pushed the fix-proj4-minus3-annie branch from 6590c66 to a74e010 Jan 7, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

Now I know some projections which don't work well with over: polar (non gctp), aeqd and polar stereographic. For polar projections you don't need over because when you wrap you end up in the same place - so that is no problem.

I think passing over to all projections should work - and it does on some machines :-)
So maybe we think about rearranging the code after we upgrade to the latest proj4 which may work more consistently.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 7, 2016

ok, but lambert fails with over as well for sure.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 7, 2016

that's a lot of elif, also with the restrcturing happening in lambert branch, it seem the gctp will not get the parameters autosets?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 7, 2016

really why not simply check against a list of proj that do not need this parameters, seems like a little more flexible, let me try to implement it in my lambert branch and lets compare ok?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

Sure, no problem.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

Does lambert (on your branch) fails only on annie or it fails everywhere?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 7, 2016

on annie and macs were the -3 tests fail. Works when -3 works.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

@doutriaux1 @aashish24 I wonder if this is a compiler issue. This is one thing that is different between annie/my laptop and my desktop. Annie has gcc 5.2.1, my laptop has llvm 7.0.2 my desktop has gcc 4.8.4. The proj4 we are using is over 10 years old so the new version should have many fixes.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 7, 2016

@doutriaux1 @aashish24 In my mind +over should be always passed as proj4 wraps around only points which create problems with the cells of the dataset. So we should revisit this after we upgrade proj4 and try to always pass +over. (for polar projection an wrapped point ends up in the same place so it should not be a difference there)

BUG: proj4 over option causes problems with polar projections
On certain machines, +over option to proj4 results in wrong
projections. It is not clear why there is a difference between different
computers - we have seen this behavior on both unix and mac. +over
does not wrap points outside of -180, 180. To fix those machines we
remove the option for polar projections.

@aashish24 aashish24 force-pushed the fix-proj4-minus3-annie branch from a74e010 to c688c62 Jan 8, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 8, 2016

@doutriaux1 can we merge this branch since its passsing on anie and other machines and then fix the lambert?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 8, 2016

@doutriaux1 I am merging this as I think it looks good to me.

aashish24 added a commit that referenced this pull request Jan 8, 2016

Merge pull request #1772 from UV-CDAT/fix-proj4-minus3-annie
BUG: proj4 over option causes problems with polar projections

@aashish24 aashish24 merged commit e3eea4b into master Jan 8, 2016

4 of 9 checks passed

cont-int/LLNL/Linux-RH6 (FULL) 'make -j15' (Thu Jan 7 20:06:22 2016)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
cont-int/LLNL/Darwin-Mac (FULL) 'In Queue: 0' (Thu Jan 7 20:04:44 2016)
Details
cont-int/LLNL/Darwin-Mac (NOGUI) 'In Queue: 0' (Thu Jan 7 20:04:43 2016)
Details
cont-int/LLNL/Darwin-Mac LEAN 'In Queue: 0' (Thu Jan 7 20:04:42 2016)
Details
cont-int/LLNL/Linux-RH6 (MESA) 'ctest -j12 -D Exper' (Thu Jan 7 20:31:25 2016)
Details
cont-int/LLNL/Linux-Ub. 15.10 (FULL/MESA) 'ctest -j15 -D Exper' (Thu Jan 7 21:00:56 2016)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aashish24 aashish24 deleted the fix-proj4-minus3-annie branch Jan 8, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 8, 2016

no non no no no NOOOOOO! It's the wrong approach I talked about it with @danlipsa this does not solve anything.... I will revert this. Beside it doesn't fix anything the issue is in proj4, it still fails on macs....

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 8, 2016

@doutriaux1 Which tests are failing? I have not seen anything in the buildbots.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 8, 2016

when i implement this in my lambert branch, the lambert test gives a different plot every time I run it.

Anyway the lambert+proj4_updated is almost done building on my mac which exposes all the issue, I have hope...

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Jan 8, 2016

@doutriaux1 @aashish24 I don't have anything against restructuring this code. If this code does not work with the lambert branch should be fixed/adjusted by that branch. I don't think that we should delay fixes for the master branch because those fixes don't fit some work in progress. If a fix passes all tests it should be good to go. Now lambert branch has to fix everything that this branch fixed plus what it was fixing. Plus we delay valid fixes for master.
In this particular case, as long as lambert goes in quickly and fixes everything, I don't have a problem with this. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment