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

Bug/sc 458619/allow negative values on most common approx #159

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

Conversation

rantolin
Copy link
Contributor

@rantolin rantolin commented Jan 8, 2025

Issue

This PR fixes these two bugs:

  1. https://app.shortcut.com/cartoteam/story/458606/error-when-nodata-value-is-assigned-to-0-in-raster-loader
  2. https://app.shortcut.com/cartoteam/story/458619/allow-negative-values-on-most-common-approx-counts

Proposed Changes

Changes fix two bugs related to computation of approximate stats:

  1. Casting to integer raises OverflowError when sum is infinity. We now catch this error and handle it.
  2. We refactored most_common_approx which is now using numpy.histogram to support the computation of approximate most common negative integer values

Pull Request Checklist

  • I have tested the changes locally
  • I have added tests to cover my changes (if applicable)
  • I have updated the documentation (if applicable)

@rantolin rantolin force-pushed the bug/sc-458619/allow-negative-values-on-most-common-approx branch from e76d76c to ec74108 Compare January 8, 2025 17:58
@rantolin rantolin requested a review from cayetanobv January 8, 2025 18:00
try:
_sum = int(np.sum(samples_band))
except OverflowError:
_sum = float("inf")
Copy link
Member

Choose a reason for hiding this comment

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

2 questions:

  • why is inf the result of the sum in that geotiff?
  • Why do we want to set the value as inf when the overflow error rises? I know that the result is inf but, is it useful? isn't it better None or zero? if you set the sum as inf in the next append iteration the result will be inf again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inf results is due to the fact that the testing geotiff has changed the nodata parameter to 0 but not having changed the actual nodata cell values that remain set to -3.4028230607370965e+38.

I have already changed this and now _sum and sum_squares are set to 0 when the sum is overflown.

@rantolin rantolin force-pushed the bug/sc-458619/allow-negative-values-on-most-common-approx branch from ec74108 to 46cab53 Compare January 10, 2025 16:24
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.

2 participants