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 area & area_percent statistics #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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