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

Fit to viewport cleanup #1806

Merged
merged 5 commits into from Feb 3, 2016
Merged

Fit to viewport cleanup #1806

merged 5 commits into from Feb 3, 2016

Conversation

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Jan 28, 2016

No description provided.

danlipsa added 3 commits Jan 25, 2016
Fail to recognize that ym,yM are not the min/max for
axis Y, but just the bounds for this axis.
Very misleading notation, by the way.
Those are margins for increasing or decreasing sequences not bounds.
So m is not less than M as suggested by the previous notation.
@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@aashish24 @doutriaux1 Now we only use dataset bounds to do the parallel projection and draw the contours and continents. Markings and labels are a little off as they still use gm bounds. We could merge this as is and start working on replacing marking and labels with VTK axis. This would save us a couple of days of work. I understand this is the plan anyway. What do you think?

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@doutriaux1 @aashish24 I am very curious about the lambert test on macs - I enable it because the failure I have seen (not the segfault) was happening because the outline and continents were off. Now this is done differently so lets see what happen.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 28, 2016

@danlipsa i would like to replace vthe whole template.plot with a real vtk backend! It would be so much nicer! But there are a LOT of subtleties in there (user custom axes labels, log scaling etc...) so we will have to be very careful when doing this.

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@doutriaux1 I am sure it won't be easy. Depending on how long it will take to add the new markings and labels in we might want to fix the labels now even if we know we are going to replace this code in the near future. I'll talk to @aashish24 about this as we have a meeting at 11.

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@doutriaux1 @aashish24 just confirmed that the VTK work is going to be longer as it involves some other things so I'll go ahead and work on fixing the labels for now.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 28, 2016

@danlipsa yes I think it's the best approach for now.

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Jan 28, 2016

@doutriaux1 Seems some mac buildbots are stuck. On one of them, the lambert test passed.

@danlipsa danlipsa force-pushed the fit-to-viewport-cleanup branch 2 times, most recently from 6288311 to 05d7469 Feb 2, 2016
@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Feb 2, 2016

@doutriaux1 @aashish24 Please review. I think the trickiest change is the one in vectorpipeline.py. In there we overlap a vector plot over a boxfill plot. Given that these have different bounds I had to adjust the viewport where the vector plot ends up so that the two plots align properly. Note the mac buildbots are stuck.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Feb 2, 2016

@danlipsa restarted macbots

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Feb 2, 2016

Thanks @doutriaux1. lambert tests look a lot better on macs with the new code. @aashish24 @doutriaux1 Are you guys OK with adding new baselines for these failures? Some labels move a little maybe because of a difference in floating point calculations or maybe of a bug in proj4. I don't think it is worth looking into why.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Feb 2, 2016

@danlipsa why are the ticks so up high on mac? (which is what the tests fail).

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Feb 3, 2016

@doutriaux1 Not sure. It may have to do with the different proj4 behavior on macs. Given that aeqd is not right anyway, I would suggest adding new baselines for these tests and taking another look with the new proj4 which I remember fixes some aeqd stuff.

@aashish24 aashish24 mentioned this pull request Feb 3, 2016
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 3, 2016

  • We will disable the troubling tests
  • We will fix the taylor plot

doutriaux1 added a commit that referenced this issue Feb 3, 2016
@doutriaux1 doutriaux1 merged commit cd00543 into master Feb 3, 2016
6 of 8 checks passed
@doutriaux1 doutriaux1 deleted the fit-to-viewport-cleanup branch Feb 3, 2016
@aashish24 aashish24 mentioned this pull request Feb 3, 2016
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Feb 3, 2016

Yay! Hopefully we won't have branches in future with so many baselines updates. I keep saying this.

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

3 participants