-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Correctly orient (y, x) arrays #1095
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1095 +/- ##
=======================================
Coverage 83.36% 83.37%
=======================================
Files 34 34
Lines 7491 7495 +4
=======================================
+ Hits 6245 6249 +4
Misses 1246 1246
Continue to review full report at Codecov.
|
Thanks Ian! I haven't dived into the changes but I remembered that some issues might have been related to this bug so tested them with this PR as it is:
Sorry for the distraction! |
@maximlt I am happy to have accidentally fixed another issue! I claim double credit! I am not surprised that the polygon/matplotlib issue is not affected as I don't think the polygon code touches anything that this PR changes. |
I think we can scratch my concerns about Holoviews output is correct. Datashader output as images ignores the attached coords so is displayed using the conventional increasing |
Very glad to see this and thanks for the detailed writeup. Also great to see that we didn't have any tests that were testing the wrong result. Overall I'd be in favor of merging this asap and then tagging a dev release so we can test this more easily. |
Self merging so we can try it out in dev release. |
Candidate fix for #1054. Surprisingly small amount of code changed in the end.
Canvas.raster
andtf.shade
both acceptxr.DataArray
s withdims[-2:] == ('y', 'x')
and the direction of each ofx
andy
coords
may be increasing or decreasing. The orientation of theDataArray
returned from these functions should be the same as the input, so if e.g. the input toCanvas.raster
is flipped in the x-direction then so will the output be. This keeps the data and coordinates in sync. This was not previously the case, the data was flipped within datashader but the coordinates weren't.Because the data and its coordinates are synchronised, you can plot the
DataArray
s inholoviews
orhvplot
and they are correctly rendered with increasing x and y.Here are two examples of this working. Firstly the OP's test case of issue #1054 using a GeoTIFF obtained from https://github.com/mommermi/geotiff_sample. The land is on the southeast, the x-coords are increasing and the y-coords are decreasing:
At the top the datashader
Image
has y-flipped as the input data does because it just draws the image data and ignores the coordinates. Below this it is plotted directly byhvplot
and also usingdatashader
which use the coordinates correctly.Second example to be completely explicit.
There are 4
DataArray
s, one for each of the 4 x/y flipping combinations.hv.Image
andshade(image)
draw them all correctly as the coordinates remain correctly oriented with respect to the data regardless of if they are passed through datashader or not. At the bottom aredatashader.Image
s that are returned fromds.Canvas.raster
followed byds.tf.shade
. The images are shown in their flipped orientation as the coordinates are ignored for rendering.One thing I have not done is consider
DataArray
s withdims == ('x', 'y')
. These are allowed but always interpreted asdims == ('y', 'x')
so strange things can result. We will need to do something about this as even some of the reproducers in issue #1054 usedims == ('x', 'y')
which made them not very helpful in diagnosing the cause of the problem. I don't think we can forceably transpose them to get('y', 'x')
but we could do some checking and either raise an exception or print a warning. But we need to be careful as we cannot just look forx
andy
but also need to considerlat
,lon
,latitude
and so on.I have kept the
calc_res
function as it is. This returns the x and y resolutions (i.e. spacings) of the specifiedDataArray
, but it chooses to reverse the yresolution. This annoys me a lot as it is unnecessary, but I have left it in case of external libraries that are using this function as it is considered public.