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

Merged

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
raster_loader/io/common.py Outdated Show resolved Hide resolved
@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
@cayetanobv cayetanobv self-requested a review January 13, 2025 18:48
@cayetanobv cayetanobv requested a review from volaya January 13, 2025 19:04
@cayetanobv cayetanobv requested a review from vdelacruzb January 14, 2025 11:37
Copy link
Contributor

@vdelacruzb vdelacruzb left a comment

Choose a reason for hiding this comment

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

LGTM

@cayetanobv cayetanobv merged commit e3bb2b1 into main Jan 14, 2025
4 checks passed
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.

3 participants