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 #1849: Re-enable datawc for linear projection. #1855

Merged
merged 2 commits into from Feb 24, 2016

Conversation

Projects
None yet
4 participants
@danlipsa
Contributor

danlipsa commented Feb 23, 2016

For datasets using a geographic projection, datawc
is only used to specify wrapping (translation of the origin,
for instance from 0:360 to -180:180) and flipping.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 23, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 24, 2016

@danlipsa why so many baselines changed? It seems to me that we are back to half cell on most of them?

@@ -690,14 +692,13 @@ def onClosing(self, cell):
if hasattr(plot, 'onClosing'):
plot.onClosing(cell)
def plotContinents(self, x1, x2, y1, y2, projection, wrap, vp, priority, **kargs):
def plotContinents(self, wc, projection, wrap, vp, priority, **kargs):

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 24, 2016

Member

good idea!

geo, geopts = project(pts, projection, getBoundsForPlotting(
[gm.datawc_x1, gm.datawc_x2, gm.datawc_y1, gm.datawc_y2], [xm, xM, ym, yM], wrap))
geo, geopts = project(pts, projection, getWrappedBounds(
[gm.datawc_x1, gm.datawc_x2, gm.datawc_y1, gm.datawc_y2], [xm, xM, ym, yM], None))

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 24, 2016

Member

why wrap None here? Does geo = None means plain array or does it means no projection? I can't remember for sure.

This comment has been minimized.

@danlipsa

danlipsa Feb 24, 2016

Contributor

This is a bug. Thanks!

y1, y2 = [y1gm, y2gm]
return [x1, x2, y1, y2]

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 24, 2016

Member

isn't it essentially what vcs.utils.getworldcoordinate does?

This comment has been minimized.

@danlipsa

danlipsa Feb 24, 2016

Contributor

@doutriaux1 @aashish24 It ends up being the same thing. Maybe instead of using getworldcoordinates which sets zooming to be datawc or the data without bounds, we should use datawc or the full data - with bounds. What do you guys think? That might cause some incompatibilities as it changes how the package behaves.

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 24, 2016

Member

i'd say it's worth trying at least.

@@ -114,7 +114,8 @@ def _plotInternal(self):
if self._maskedDataMapper is not None:
self._mappers.insert(0, self._maskedDataMapper)
x1, x2, y1, y2 = self.getBoundsForPlotting()
plotting_dataset_bounds = self.getPlottingBounds()
x1, x2, y1, y2 = plotting_dataset_bounds

This comment has been minimized.

@doutriaux1

doutriaux1 Feb 24, 2016

Member

do we still need x1,x2,y1,y2?

This comment has been minimized.

@danlipsa

danlipsa Feb 24, 2016

Contributor

Yes, they are still used a little lower.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

@danlipsa looks good, let's add a quick test to make sure we don' break this again.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@aashish24 @doutriaux1 It is true that we are back to half cell - this is what vcs.utils.getworldcoordinate tells us to do - to zoom into the dataset.

We can try to get rid of vcs.utils.getworldcoordinate and only follow datawc. getworldcoordinates sets zooming based on the datawc or if that is not set it sets it based on the data (without bounds).

Note there is still no cutting or misalignment so we did not loose that.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

@aashish24 where do you see a half cell? worldcoordinate uses your axes values, we can try to tweak it to use the axes bounds rather than the just the axes. I'm opened to that.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 Look for instance at https://github.com/UV-CDAT/uvcdat-testdata/pull/112/files. Instead of showing the whole dataset we zoom in and cut the cells on the margine in half.
Take a look at infinity.png for instance.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

Ok I found a few ones finally. Thanks. Let's try to have getworldcoordinate return the bounds. Historically it use to be just the axes and originally I wanted to preserve this, to help users feel more confident with the VTK transition. At this point I don't think we need to do this anymore. Let's have getworldcoordinate return the real world coordinates of the data, i..e with axes bounds.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 @aashish24 The problem with modifying getworldcoordinates is that coordinates depend on the type of plot we have: boxfill uses bounds, isofill, isoline and vector do not.
Question: Why do we create both cell data and point data datasets? This does not seem to be specified in the nc file.
Regardless, it is possible for different variables to have different bounds coming from an nc file in which case there is nothing we can do.

I already started passing backend.dataset_bounds using **kargs mechanism. I would use that instead and maybe get rid of getworldcoordinates alltogether as some point. Maybe we do this in two steps. 1. Try to show the full dataset if datawc is not set. I will use dataset_bounds for this 2. get rid of getworldcoordinates all together as a separate issue.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

@danlipsa the worldcoordinates for all graphic method should come from bounds, in the case of meshfill there are no axes so use data.getMashfill().min/max()
boxfill/meshfill uses celldata because we draw cells/polygones defined by the data itself, isolines and isofill only need point data since we interpolate. Does that make sense?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 I was thinking that always using cell data would make the code more consistent and simplify it. If we need point data in certain plots we can easily convert cell data to point data. So, we always create the data that is specified in the file, regardless of what a specific plot needs. Note that vtkCellDataToPointData works by averaging cells used by every point, which will give you the same bounds as for the cell data which is a nice thing to have when doing several plots in the same image.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

@danlipsa that's fine with me. I thought isofill was requiring point data (filter) if it doesn't then it's ok with me. Although point data is nice for unstructured grid since we don't always have the whole cell information. Anyhow let's do this in a separate PR.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 Definitely a separate PR.

@danlipsa danlipsa force-pushed the respect_zoom branch from f968591 to c36c952 Feb 24, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@aashish24 @doutriaux1 Note there are missing triangles when zooming into mesh data:
vcs_test_meshfill_zoom
Probably has to do with wrapping of meshfill data, but I don't know why it happens. I will look into it.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 besides that problem it should be ready to merge. I think it is worth trying to see if it fixes PCMDI/pcmdi_metrics#296. @aashish24 We decided to leave the half cells in as this was the original behavior of uvcdat and we'll be faster in fixing the issue at hand. We can enable showing the whole dataset as a separate PR.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

@danlipsa yes! It does fix this issue:
test_4seasrms

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 Great!

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 Interesting plot by the way.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 24, 2016

@doutriaux1 What does the plot mean? For each cell, it shows 4 things? What are those? I am curious. :-)

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 24, 2016

Each cell is basically a score skill for a model/variable. The cell itself is broken into N (4 in this case) references. That is to make sure your score does not depend on the set set of observation you used.

danlipsa added some commits Feb 23, 2016

BUG #1849: Re-enable datawc for linear projection.
For datasets using a geographic projection, datawc
is only used to specify wrapping (translation of the origin,
for instance from 0:360 to -180:180) and flipping.

@danlipsa danlipsa force-pushed the respect_zoom branch from c36c952 to 0578299 Feb 24, 2016

doutriaux1 added a commit that referenced this pull request Feb 24, 2016

Merge pull request #1855 from UV-CDAT/respect_zoom
BUG #1849: Re-enable datawc for linear projection.

@doutriaux1 doutriaux1 merged commit 39a367c into master Feb 24, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@durack1

This comment has been minimized.

Member

durack1 commented Feb 25, 2016

@danlipsa, @gleckler1 will be delighted that you think the gleckler plot is "interesting".. He's going to be making many more of them in the coming months..

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Feb 26, 2016

@durack1 Indeed, I think the plot looks good - it draw me in even without knowing what it means.

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