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

Handle mercator infinity projection at 90,-90 #1552

Merged
merged 11 commits into from Sep 21, 2015

Conversation

Projects
None yet
4 participants
@aashish24
Contributor

aashish24 commented Sep 17, 2015

Not ready for review yet.. just wanted to push for buildbot.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

Ref #1552

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 17, 2015

130 or so failures! Wow congrats @aashish24 😉

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 17, 2015

LGTM merging. 😉

aashish24 added some commits Sep 17, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@doutriaux1 yes, we should make a rule to merge only those branches that can bring atleast 100+ test failures -:).

Ready for review. One thing I noted (which I am not sure if caused by my branch), is that the text in the rendered plot overlaps slightly with the visualization. I am not sure if that is something related.
@@ -94,11 +94,27 @@ def putMaskOnVTKGrid(data, grid, actorColor=None, cellData=True, deep=True):
grid.GetPointData().AddArray(ghost)
return mapper
def handleProjectionEdgeCases(projName, data):
# For mercator projection, latitude values of -90 or 90
# transformation result in infinity values. We chose -85, 85

This comment has been minimized.

@jbeezley

jbeezley Sep 17, 2015

Contributor

Technically, 85.051129 is used because that makes the domain a square in projected coordinates.
https://en.wikipedia.org/wiki/Web_Mercator#Formulas

This comment has been minimized.

@aashish24

aashish24 Sep 17, 2015

Contributor

@jbeezley sure, but matplotlib uses 85.0 and so as some other tools. For UV-CDAT since we scale the plot, which makes it a rectangle anyways but I am willing to change it to 85.051129 if that is preferred.

This comment has been minimized.

@jbeezley

jbeezley Sep 17, 2015

Contributor

No 85.0 is close enough for any practical purpose, I was just pointing out that it isn't a magic number chosen arbitrarily. It is derived for a reason.

This comment has been minimized.

@aashish24

aashish24 Sep 17, 2015

Contributor

sure, yup, I knew -:) but you are right not everyone would know. On a related note, matplotlib has a threshold option as well that I would like to add at some point.

This comment has been minimized.

@aashish24

aashish24 Sep 17, 2015

Contributor

On this, I would like to propose that VCS do not scale the viewport but that's something I need to bring up in some meeting. The idea is to fill the screen but to me anytime you use a projection, it really comes out distorted.

This comment has been minimized.

@jbeezley

jbeezley Sep 17, 2015

Contributor

👍 The aspect ratio of the projection should always be 1:1.

@aashish24 aashish24 force-pushed the fix_mercator branch from c7961f7 to a0166fe Sep 17, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

Ref: #587

@aashish24 aashish24 force-pushed the fix_mercator branch from a0166fe to 8bd47f6 Sep 17, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@doutriaux1 some of tests are failing because of the bad testdata branch. I repushed it.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 17, 2015

@aashish24 one test failed and no flake8.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 17, 2015

@aashish24 I'm fixing annie should be able to run the build soon after is merged #1551

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@doutriaux1 all of the tests should pass now.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@doutriaux1 ready to be merged.. all tests are passing.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 17, 2015

@doutriaux1 looks like RH6 is not pulling the right branch for test-data

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 17, 2015

@aashish24 I wonder if ctest forces it back to master branch? I will take a look. The history show a successful switch to fix_mercator branch.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 18, 2015

@doutriaux1 I am going to merge this branch soon since all of the tests are passing now.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 18, 2015

Please keep it open I will merge it. I need to figure out why the bots don't check the correct branch

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 18, 2015

@aashish24 seems like my bots are out of sync with the master branch, going to update them and retrigger the builds. Please hold on.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 18, 2015

@doutriaux1 ping!!

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 18, 2015

let me try locallly

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 18, 2015

x = vcs.init()
iso = x.createisofill()
iso.projection = "mercator"
x.plot(s(latitude=(-90, 90)), iso)

This comment has been minimized.

@doutriaux1

doutriaux1 Sep 18, 2015

Member

@aashish24 please use setbgdimension and plot in bg=1

This comment has been minimized.

@doutriaux1

doutriaux1 Sep 18, 2015

Member

I suspect the slight diff on my ubunutu maybe due to default size being to small.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 21, 2015

@doutriaux1 I have pushed changes as suggested. I may have to upload a new baseline as well.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 21, 2015

@aashish24 thanks

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Sep 21, 2015

@doutriaux1 ready for your review.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Sep 21, 2015

I don't understand why the bots do not check the correct uvcdat-testdata, running the command locally works...

doutriaux1 added a commit that referenced this pull request Sep 21, 2015

Merge pull request #1552 from UV-CDAT/fix_mercator
Handle mercator infinity projection at 90,-90

##bot##skip-commit

@doutriaux1 doutriaux1 merged commit 931d41e into master Sep 21, 2015

3 of 7 checks passed

cont-int/LLNL/Darwin-Mac 10.10.3 running 'ctest -j4 -D Experimental' (Mon Sep 21 15:21:45 2015)
Details
cont-int/LLNL/Linux-annie Ubuntu 14.04 (FULL/MESA) running 'ctest -j15 -D Experimental' (Mon Sep 21 13:46:54 2015)
Details
cont-int/LLNL/Linux-crunchy RH6 (FULL) running 'ctest -j12 -D Experimental' (Mon Sep 21 13:07:36 2015)
Details
cont-int/LLNL/Linux-oceanonly RH6 (MESA/NOGUI) running 'ctest -j12 -D Experimental' (Mon Sep 21 13:04:38 2015)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@doutriaux1 doutriaux1 deleted the fix_mercator branch Sep 21, 2015

@durack1

This comment has been minimized.

Member

durack1 commented Sep 22, 2015

@doutriaux1 maybe get the ENV to be printed to the log to see what the problem is..

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