Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

223 jg subset spatial fix #295

Merged
merged 22 commits into from
Jul 19, 2017
Merged

223 jg subset spatial fix #295

merged 22 commits into from
Jul 19, 2017

Conversation

JanisGailis
Copy link
Member

A uniform way for handling updates to global attributes along with associated changes in operations, tests and documentation.

Closes #294
Closes #223

Janis Gailis added 12 commits July 11, 2017 16:34
Module name will automatically be added to the tag list provided by
user
Account for the case where the user has already provided a tag of the
same name as the module name.
There are now tests for adjustment functions, as well as some fix due to
testing.
@JanisGailis JanisGailis added this to the IPM6 milestone Jul 13, 2017
@JanisGailis JanisGailis self-assigned this Jul 13, 2017
@JanisGailis JanisGailis requested review from mzuehlke and forman July 13, 2017 15:16
@codecov-io
Copy link

codecov-io commented Jul 13, 2017

Codecov Report

Merging #295 into master will increase coverage by 0.78%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   71.44%   72.23%   +0.78%     
==========================================
  Files          70       71       +1     
  Lines        9869    10239     +370     
==========================================
+ Hits         7051     7396     +345     
- Misses       2818     2843      +25
Impacted Files Coverage Δ
cate/ops/correlation.py 98.73% <100%> (+0.01%) ⬆️
cate/ops/aggregate.py 100% <100%> (ø) ⬆️
cate/ops/coregistration.py 97.87% <100%> (-0.11%) ⬇️
cate/ops/subset.py 97.4% <100%> (+0.03%) ⬆️
cate/ops/normalize.py 96.66% <94.91%> (-1.7%) ⬇️
cate/core/op.py 90.27% <0%> (-1.56%) ⬇️
cate/core/plugin.py 75.75% <0%> (-0.72%) ⬇️
cate/util/misc.py 87.71% <0%> (-0.42%) ⬇️
cate/util/process.py 89.76% <0%> (-0.39%) ⬇️
cate/core/workspace.py 65.53% <0%> (-0.25%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc419f4...35c57cc. Read the comment docs.

@JanisGailis
Copy link
Member Author

JanisGailis commented Jul 13, 2017

For some reason the auto-tag stuff has leaked into this. I'll try to weed it out somehow.

I guess I git checkout -b <new> from that branch.

Janis Gailis added 3 commits July 13, 2017 18:36
@JanisGailis
Copy link
Member Author

git revert --no-commit does wonders.

@forman
Copy link
Member

forman commented Jul 14, 2017

Hi Janis, the computation of spatial boundaries is not exact. We must take grid cell boundaries into account. Each value in the lon, lat coordinate variables usually refer to cell centers, especially if the coordinate variables lon_bnds, lat_bnds are given. Their elements are pairs that provide each cell's lower and upper bound. If they are given, then the outer limits are given by

lat_min = min(lat_bnds[0][0], lat_bnds[-1][1])
lat_max = max(lat_bnds[0][0], lat_bnds[-1][1])

If not, the boundaries are

lat_res = abs(lat[1] - lat[0])
lat_min = min(lat[-1], lat[0]) - 0.5 * lat_res
lat_max = max(lat[-1], lat[0]) + 0.5 * lat_res

equidistant grids presumed.

There is already code that computes boundaries as described: workspace.py

@JanisGailis
Copy link
Member Author

Do you have a conventions reference for this?

I've seen CCI datasets that treat lat_min pointed to a pixel boundary, but I've also seen plenty pointing to the actual 'minimum' of the coordinate vector.

@forman
Copy link
Member

forman commented Jul 14, 2017

It is in the CF convention. As most CCI datasets apply them you'll find lat[0] = 90 - 0.5 * res and lat[-1] = -90 + 0.5 * res, so that the boundaries are -90, +90 for global coverage.

@forman
Copy link
Member

forman commented Jul 14, 2017

but I've also seen plenty pointing to the actual 'minimum' of the coordinate vector.

Do you remember which ones?

@JanisGailis
Copy link
Member Author

You mean lat[0] would be something like -180 and lat[-1] would be 180? I think that's the case in all datasets we manage to open. In fact, we had a problem with data access when that's not the case, so I think all of them are now reversed to that, if not already the case.

@forman
Copy link
Member

forman commented Jul 14, 2017

No!

@forman
Copy link
Member

forman commented Jul 14, 2017

Lat cannot be 180 :)

@JanisGailis
Copy link
Member Author

Yes, -90, 90 :)

@forman
Copy link
Member

forman commented Jul 14, 2017

I fact, I meant the opposite, lat[0] = 90. That means, if you display the grid as an image you would see in in the correct orientation with N up, S down.

@forman
Copy link
Member

forman commented Jul 14, 2017

We have both cases in CCI

@JanisGailis
Copy link
Member Author

Well, yes, we don't handle lat[0]=90 grids well. We even had an issue on this. I think we're reversing them now upon access.

@forman
Copy link
Member

forman commented Jul 14, 2017

Btw, my example above is a bad one as it uses 2D lat, lon coordinate arrays.

@mzuehlke
Copy link
Collaborator

oc-cci has it's lat values going from +90 to -90

@forman
Copy link
Member

forman commented Jul 14, 2017

Which I like

@JanisGailis
Copy link
Member Author

From
From esacci.CLOUD.mon.L3C.CLD_PRODUCTS.multi-sensor.multi-platform.ATSR2-AATSR.2-0.r1

    geospatial_lon_resolution:   0.50
    geospatial_lat_resolution:   0.50
    geospatial_lat_min:          -89.75
    geospatial_lat_max:          89.75
    geospatial_lon_min:          -179.75
    geospatial_lon_max:          179.75
    geospatial_lat_units:        degrees_north
    geospatial_lon_units:        degrees_east
    geospatial_vertical_min:     0.0
    geospatial_vertical_max:     0.0

@forman
Copy link
Member

forman commented Jul 14, 2017

That's definitely wrong!

@forman
Copy link
Member

forman commented Jul 14, 2017

Or it is correct, because they cut off half a grid cell at both ends for some reason (e.g. anti-meridian problem, he, he).

@JanisGailis
Copy link
Member Author

OK, yes, I agree, if that denotes a bounding box of the dataset, then it shouldn't cut pixels. As simple as that :)

@JanisGailis
Copy link
Member Author

See: #196

@forman
Copy link
Member

forman commented Jul 14, 2017

Yepp.

@JanisGailis
Copy link
Member Author

Yes, we don't reverse it upon reading, sm data goes from 90 to -90:

 * lat      (lat) float32 16.875 16.625 16.375 16.125 15.875 15.625 15.375

OK, this is good to realize (again, I guess), as that might make a difference in some ops.

Cool, then I'm off to implementing the changes you suggested! Thanks for taking the time to look at the PR! :)

@JanisGailis
Copy link
Member Author

@forman @mzuehlke Good to go now?

@forman
Copy link
Member

forman commented Jul 19, 2017

Great

@forman forman closed this Jul 19, 2017
@forman forman reopened this Jul 19, 2017
@forman forman merged commit 504e75e into master Jul 19, 2017
@JanisGailis JanisGailis deleted the 223-jg-subset-spatial-fix branch September 7, 2017 11:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants