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 Force() to force computation of histogram if not cached #63

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tbonfort
Copy link
Member

No description provided.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1602495501

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0008%) to 99.648%

Totals Coverage Status
Change from base Build 1520238762: 0.0008%
Covered Lines: 2550
Relevant Lines: 2559

💛 - Coveralls

@tbonfort
Copy link
Member Author

cc @fhelen for the addition of Force() to be reused by Statistics()

@tbonfort
Copy link
Member Author

cc @thomascoquet : this should fix the issue you raised in #60 about Histogram() forcing a full scan.

@tbonfort
Copy link
Member Author

@thomascoquet using Histogram(Intervals(count,min,max)) will always force a dataset scan. Should we require a user to write Histogram(Intervals(count,min,max), Force()), i.e. return an error if Force is not provided?

@thomascoquet
Copy link
Contributor

Sorry @tbonfort, I missed the GitHub notification in my mails. I agree it is better to have an adequate flag such as Force() ; last thing one wants is a unintentional remote read.


I think it is a little bit obscure how to properly use histograms in godal. Indeed, there are two ways to compute histograms:

A very (and maybe the most) frequent use case for histograms is what I would call the "QGIS" use case: there is a raster you know nothing about, you need to display some color palette on it and apply some percentile-based stretching. In which case you would like the cached histogram or a cheap approximated version.

The most natural way is to call Histogram() and maybe Histogram(Approx(), Force()) to allow computation on overviews if not cached. With the current flow, Histogram(Approx(), Force()) will never consider the Approx option and will read the raster at full resolution.

Therefore, I would return an error in this PR if both options are used and wait for @fhelen to solve this using approximated statistics.

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