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

Issue 1763 lambert #1768

Merged
merged 20 commits into from Jan 12, 2016
Merged

Issue 1763 lambert #1768

merged 20 commits into from Jan 12, 2016

Conversation

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 6, 2016

No description provided.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@aashish24 please review. Failing test are due to the proj4 bug.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@sankhesh @danlipsa @chaosphere2112 please review as well. proj4 bug is causing failures.

projName = pname
pd.SetName(projName)
if projection.type == 'aeqd':
# this is a temporary branch to keep the same
Copy link
Contributor

@aashish24 aashish24 Jan 7, 2016

this comment seems absurd

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 7, 2016

I agree but since it was here before I left it in. I might mean something to someone...

Copy link
Contributor

@aashish24 aashish24 Jan 7, 2016

I would say remove it. Its not useful anymore.

if ((not hasattr(projection, 'centralmeridian') or
numpy.allclose(projection.centralmeridian, 1e+20))):
pd.SetCentralMeridian(float(xm + xM) / 2.0)
apply_proj_parameters(pd, projection, xm, xM, ym, yM)
Copy link
Contributor

@sankhesh sankhesh Jan 7, 2016

👍 Good to remove code duplication

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@aashish24 ok will remove comments and repush

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@aashish24 pushed. Please review again and merge at your convenience

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 7, 2016

thanks @doutriaux1 looks great! Also thanks for removing duplicated code BIG 👍 from me.

LGTM 👍

since proj4 has issue with it on some systems
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@aashish24 I'm adding the bit to turn off over parameter please wait before merging

made test pass on my weird 3 screen system
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

@aashish24 tests seem to now pass on my mac.

optional param over needs to ACTIVELY be set to false in order to get rid of the bug
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 7, 2016

@doutriaux1 I am wondering why other bots are stuck?

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 7, 2016

me too... I keep restarting them... something to do with the Queueing system when a new PR goes to the Queue it seems to seriously mess up the macs especially... Had to kill everything a few times...

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 8, 2016

wow... On my mac I HAD TO set "over" to false in order to consistently get correct result for lambert plots otherwise i got a completely different plot every time. Now that this is in it seems to have broken Ubuntu/MESA... I'm at a loss here!! @danlipsa @aashish24 please take over that branch and let me know..

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 8, 2016

@doutriaux1 @danlipsa I have added support newer proj4 (#1773), as of writing this I am still waiting on dashbords to finish.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 8, 2016

Here is the corresponding VTK pull request: https://github.com/UV-CDAT/VTK/pull/14/files

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 8, 2016

That is great when newer proj4 is in let me update this PR so that over iscalways on.

doutriaux1 added 2 commits Jan 9, 2016
cannot do a global lambert it makes no sense
lambert breaks on apple
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 10, 2016

@aashish24 @danlipsa @chaosphere2112 I'm done with the code, I merged the noticklabel elliptical branch in this so we only need to update the baseline once. It's mostly there but animation baselines need update and a few others. I probably won't have time to look into this tomorrw, so feel free to at least review the code now and if you could update the baselines it would be awesome to! Thanks.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 11, 2016

@doutriaux1 the polar tests are failing: Look here:

https://open.cdash.org/testSummary.php?project=44&name=vcs_test_animate_projected_isofill_polar&date=2016-01-10

I am not even sure what the test is testing as the baseline itself is looking pretty bad and the new image is worse.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 11, 2016

I didn't look at animation test. I'll look in the morning.

@@ -21,6 +21,9 @@
round_projections = ['polar (non gctp)', 'stereographic',
'orthographic', "ortho", ]

no_over_proj4_parameter_projections = round_projections+["aeqd", "lambert conformal c"]
Copy link
Contributor

@danlipsa danlipsa Jan 11, 2016

In my original pull request I also did not pass +over for 'polar stereographic'. Maybe this is the reason why this you are failing some polar tests. Besides that looks good to me.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 11, 2016

shouldn't be, because polar do not go to the part of the code that uses no_over_proj4_parameter_projections I will take a look at the animation test now.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 11, 2016

ok i see the issue, the parameter passed to the projection should not be polar, it should be polar (non gctp) or -3

The animation test are sol long I'm removing the polar projection from anim test. I don't think animating many projections is useful anyway, one test on an animated projection is enough.

Copy link
Contributor

@aashish24 aashish24 Jan 11, 2016

The animation test are sol long I'm removing the polar projection from anim test. I don't think animating many projections is useful anyway, one test on an animated projection is enough.

Sounds good but make sure that other polar projections are still working.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Jan 11, 2016

I did, they work, or at least they work so that they do not exhibit the bug...

projection have their own tests, one test of a animated plot with projection should be enough
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 @chaosphere2112 @danlipsa I think we should be good know, let's see what the bots think.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

OK rh full issues are most likely vnc server issues, the failed tests are the ones that open a window to the screeb

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

Also, @doutriaux1 if you can please squash your commits or update the comments: For example: even height could use more words.

Other than that, its looks okay to me. I am going to test the projection stuff some more post 2.4.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 updating files for Mac failures. Test failure look very similar to vtkweb issue. Not a show stopper, but I will double check it tomorrow. Will try to squash but probably won't get to it until tomorrow.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish crunchy is acting up and has been started over vpn and xfvb I'm not too concerned about the test failure. Updated Mac baselines. Restarting bots to make sure it's all green.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 why aren't your machine ctest results visible?

@jbeezley
Copy link

@jbeezley jbeezley commented Jan 12, 2016

@doutriaux1 There was a timeout cloning a repo during the build. I just retriggered it.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@jbeezley thx! My mac bot also got hanged, restarting them now...

@doutriaux1 doutriaux1 force-pushed the issue_1763_lambert branch from 6a17ae3 to 3d751cc Jan 12, 2016
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 squashed commit and retriggered build for macs. Let's merge as soon as possible, double checking the crunchy issue

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 macs are happy now, Ubunutu and RH6 as well. We should merge so we can tag 2.4.0

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

@doutriaux1 looking into it right now.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 thx!

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

@doutriaux1 Looks good thanks for updating the commits.

aashish24 added a commit that referenced this issue Jan 12, 2016
@aashish24 aashish24 merged commit 6e7db69 into master Jan 12, 2016
6 of 8 checks passed
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

@doutriaux1 you are welcome. Okay if I go ahead and create and tag the release branch? like we discussed last time.

@doutriaux1 doutriaux1 deleted the issue_1763_lambert branch Jan 12, 2016
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Jan 12, 2016

@aashish24 let's wait a couple of days for the tag I would like @williams13 and @potter2 to take a look first.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 12, 2016

@aashish24 let's wait a couple of days for the tag I would like @williams13 and @potter2 to take a look first.

Okay, will wait. But if that process takes longer than 1-2 days we would have to start merging other pending work (or new work) in master. We can always pick a older sha for release so that's not a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants