Skip to content

Conversation

@diversoft
Copy link

No description provided.

@perrygeo
Copy link
Owner

@diversoft thanks for the PR. Two comments:

  • Can you add some tests to cover the new functionality?
  • There was a rasterio 1.0 compatibility problem that was causing test failures. It's fixed now in master, the tests should run properly on your next commit.

if rast.src is not None:
cellsize = rast.src.res[0] * rast.src.res[1]
else:
cellsize = rast.affine.a * rast.affine.a
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
cellsize = rast.affine.a * rast.affine.a
cellsize = rast.affine.a * rast.affine.e

Copy link
Owner

Choose a reason for hiding this comment

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

The cells size shouldn't be assumed to be square. It's very rare in practice to see rasters where a != e but it could happen

Copy link

Choose a reason for hiding this comment

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

I think you need the absolute value here: abs(rast.affine.a * rast.affine.e)

Copy link
Contributor

Choose a reason for hiding this comment

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

To be safe, I would recommend reprojecting to be sure. I implemented a utility function for this use-case here - https://github.com/WFP-VAM/prism-app/pull/893/files#diff-0e7a305eb2d7da6652444269a99141f2a18d2fb8ea0ddbc045cd6c5a16e2ee9bR104

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants