Skip to content

Comments

Warper: Add TIE_STRATEGY warp option#11649

Merged
rouault merged 1 commit intoOSGeo:masterfrom
dbaston:wo-tie-strategy
Jan 16, 2025
Merged

Warper: Add TIE_STRATEGY warp option#11649
rouault merged 1 commit intoOSGeo:masterfrom
dbaston:wo-tie-strategy

Conversation

@dbaston
Copy link
Member

@dbaston dbaston commented Jan 14, 2025

What does this PR do?

Adds a TIE_STRATEGY warping option to provide some control over the destination pixel value during "mode" resampling when multiple source values appear at the same frequency. The current behavior of choosing the first value encountered can introduce a spatial bias (favoring values from the north/west) that is undesirable in some applications.

What are related issues/pull requests?

Tasklist

  • Review
  • Adjust for comments
  • All CI builds and checks have passed

static_cast<int>(dfValueRealTmp);
if (++panVals[nVal + nBinsOffset] > nMaxVal)

bool bValIsMax =
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't due to your changes, but the current variable naming is terrible and makes it hard to follow the algorithm. Would you mind renaming:

  • panVals to something like panCount
  • nMaxVal to nMaxCount
  • iMaxInd to nModeValCandidate ?
  • bValIsMax to bValIsMaxCount

I believe panRealSums could be removed, and just replaced by panCount shared between int and float implementations

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the variable names caused me some confusion when working on this. I'll try to improve them in a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I was surprised to see that "mode" resampling isn't accounting for pixel coverage fractions in the way that "sum" and "average" are. I'm planning to address this as I rename the variables.

Copy link
Member

Choose a reason for hiding this comment

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

I'm planning to address this as I rename the variables.

yes, that would definitely make sense for "mode" as well

@rouault
Copy link
Member

rouault commented Jan 14, 2025

Looks reasonable to me, but if the ties are determined by the value of the samples, this implies that the classification is somehow sorted (like "CO2 rate [X-Y ppm]", "CO2 rate [Y-Z ppm]", etc ...) ? If so, wouldn't something more appropriate be to compute the average of the contributing samples and select the value in the contributing pixels which is the closest to that average?

No need to generalize that to overview computation ? If so we could rather need new GRIORA_ or GRA_ enumerations rather than a warping option

@dbaston
Copy link
Member Author

dbaston commented Jan 14, 2025

if the ties are determined by the value of the samples, this implies that the classification is somehow sorted

I'm using this with a land cover data. So while the values are not naturally sorted, I can reclassify them in a way that MIN or MAX will do what I want.

wouldn't something more appropriate be to compute the average of the contributing samples and select the value in the contributing pixels which is the closest to that average?

I can imagine adding such an option, e.g. TIE_STRATEGY=CENTRAL or something.

@rouault
Copy link
Member

rouault commented Jan 14, 2025

I can imagine adding such an option, e.g. TIE_STRATEGY=CENTRAL or something.

I wasn't asking for it. Just thinking out loud and wondering if it would make some sense, but I'm not sure if there's a real use case. From your comment about your use case, it seems not

@dbaston
Copy link
Member Author

dbaston commented Jan 15, 2025

Is MODE_TIES a better name for this option?

MODE_TIES=FIRST
MODE_TIES=MIN
MODE_TIES=MAX
MODE_TIES=3,7,2 (a colleague requested to manually spell out prioritization -- would do in a separate PR)

@rouault
Copy link
Member

rouault commented Jan 15, 2025

Is MODE_TIES a better name for this option?

sounds reasonable

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 70.063% (+0.01%) from 70.053%
when pulling a803e49 on dbaston:wo-tie-strategy
into 4f6fde6 on OSGeo:master.

@rouault rouault merged commit 94e69ab into OSGeo:master Jan 16, 2025
37 of 38 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