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

Add support for gridspec origin convention #158

Merged
merged 1 commit into from
Oct 4, 2016
Merged

Add support for gridspec origin convention #158

merged 1 commit into from
Oct 4, 2016

Conversation

ceholden
Copy link
Contributor

This addition follows the addition of origin to the GridSpec configuration in helping to make tiled data output from the ingestion compatible with existing gridded data. The origin specification allows the data to co-align and this proposed change would also allow the tile indexes to be identical by allowing users to count indexes relative to an upper left instead of the current bottom left. My interest in this change comes from wanting our tiled data to be in line with data from the Landsat WELD project grid specification (which IIRC is also being used for the USGS LCMAP project).

I've added a test that was validated against the WELD tile lookup calculator, and I've also run an ingest with the proposed changes. The tile indices created from this ingest run using convention = "upperleft" match and since the origin addition the tiles also coalign. The current "convention" of counting from the bottom left is still default and I don't think I've affected any calculations for that case.

Happy to iterate on any aspect of this with you all. Thanks!

@ceholden
Copy link
Contributor Author

Ooops -- I pushed over the 1000 line/module limit:

+pylint -j 2 --reports no datacube datacube_apps
************* Module datacube.model.__init__
C:  1, 0: Too many lines in module (1009/1000) (too-many-lines)

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.02%) to 79.492% when pulling 5d8aa289828665c69fba00f13b1fa9964b4b36dc on ceholden:develop into dddd3f7 on data-cube:develop.

@v0lat1le
Copy link
Contributor

I think the tile pixel coordinates (GeoBox) are not calculated correctly. GridSpec.tile_coords needs to take conventions into account. I've updated test_grispec test to check the geobox coordinates.

I'm also wandering if the conventions can be controlled with a sign on tile_size in the config... Minus to indicate that tile index increases in negative direction of the coordinate.

@ceholden ceholden closed this Sep 14, 2016
@ceholden
Copy link
Contributor Author

@v0lat1le I have some initial work toward specifying upper-left via a negative y/latitude in tile_size and it seems to be working out. IMO this is a much more preferable solution than introducing another configuration option, so I'll reopen this PR when I have something working.

@ceholden ceholden reopened this Sep 20, 2016
@coveralls
Copy link

coveralls commented Sep 20, 2016

Coverage Status

Coverage decreased (-0.1%) to 79.364% when pulling 4eac0f933b3d18238dd16665317dccc1bf47db1c on ceholden:develop into 8b00217 on data-cube:develop.

@coveralls
Copy link

coveralls commented Sep 20, 2016

Coverage Status

Coverage decreased (-0.1%) to 79.364% when pulling 4eac0f933b3d18238dd16665317dccc1bf47db1c on ceholden:develop into 8b00217 on data-cube:develop.

@@ -718,8 +718,11 @@ def grid_range(lower, upper, step):
>>> list(GridSpec.grid_range(1.0, 4.0, 3.0))
[0, 1]
"""
assert step > 0.0
return range(int(math.floor(lower / step)), int(math.ceil(upper / step)))
assert abs(step) > 0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert fails:

assert list(GridSpec.grid_range(1.0, 4.0, -3.0)) == [-2, -1]

Having something like this an the top of the function should do the trick:

if step < 0.0:
    lower, upper, step = -upper, -lower, -step

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much smarter than what I was doing. New commit adds that example in doctest format and I switched to the implementation you suggested.

Negative y dimension for tile_size combined with origin
specified for upper-left allows tile indexes to be consistent
with other grid specifications like WELD or MODIS
@coveralls
Copy link

coveralls commented Sep 30, 2016

Coverage Status

Coverage increased (+0.007%) to 80.178% when pulling d502ed7 on ceholden:develop into 6ada94d on data-cube:develop.

@ceholden
Copy link
Contributor Author

ceholden commented Oct 2, 2016

I finally got back around to this again and implemented the change as was suggested in the PR review. The examples I added specific to WELD's grid in the tests and the GridSpec.grid_range doctests pass. I also went through an ingest of two Landsat path/rows and everything looks great! Thanks!

@v0lat1le v0lat1le merged commit c4516bf into opendatacube:develop Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants