-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
VRT Processed Dataset: Add BandScaleOffset algorithm #11573
base: master
Are you sure you want to change the base?
Conversation
@@ -574,6 +574,9 @@ CPLErr VRTProcessedDataset::Init(const CPLXMLNode *psTree, | |||
auto poBand = | |||
new VRTProcessedRasterBand(this, i + 1, eOutputBandType); | |||
poBand->CopyCommonInfoFrom(poSrcBand); | |||
poBand->SetNoDataValue(adfNoData[i]); | |||
poBand->SetScale(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we would want to copy the scale and offset from poSrcBand
if we are not using the BandScaleOffset
algorithm, but making this behavior conditional seems messy and I'm not sure there is a use case for applying algorithms to scaled values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, if we say (as these two lines imply) that algorithms must be run on the unscaled data, is the BandScaleOffset
algorithm desirable at all? Or should the band scale/offset just be applied automatically when reading from the source raster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should the band scale/offset just be applied automatically when reading from the source raster?
yes I guess applying it automatically should satisfy 99% of use cases. Do we want to provide some option in the XML to disable the automatic unscaling ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unscale" attribute of "Input" with default value of "true" ? So <Input unscale="false" />
if you want to preserve the current behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that sounds good.
auto data = std::make_unique<BandScaleOffsetData>(nInBands, padfInNoData, | ||
papszFunctionArgs); | ||
|
||
// Input NoData value is meaningless after scaling, so we set the output NoData value to NaN. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure using NaN
is appropriate here or if I need to look elsewhere for a declared NoData value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BandAffineCombination has an optional
" <Argument name='dst_nodata' type='double' "
"description='Override output nodata value'/>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add that here?
I don't have a strong opinion if we need to provide a override dstnodata. Just documenting that the algorithm standardize on NaN should be fine
7d68111
to
151ad77
Compare
@@ -12,6 +12,9 @@ to apply chained processing steps that may apply to several bands at the same ti | |||
The following built-in algorithms are introduced, and may typically be applied | |||
in the following order: | |||
|
|||
- BandScaleOffset (since GDAL 3.11): apply per-band scale and offset values from | |||
:cpp:func:`GDALRasterBand::GetScale` and :cpp:func:`GDALRasterBand::GetOffset`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an example would be welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example could describe how to set back the scale and offset values of the input data, which were "unscaled" earlier.
User may have data like the recommended DGIWG DEM variant that is scaled into Int16, and it may be surprise that data gets automatically unscaled, but it must be explicitly scaled back if Int16 with scale/offset is still desired.
I apologize if I have misunderstood something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it may be surprise that data gets automatically unscaled, but it must be explicitly scaled back if Int16 with scale/offset is still desired
I think a challenge with automatically re-scaling is knowing what scale factor to use. If we've applied an arbitrary processing algorithm to the inputs, I don't think we can assume the input scale/offset are still appropriate.
auto data = std::make_unique<BandScaleOffsetData>(nInBands, padfInNoData, | ||
papszFunctionArgs); | ||
|
||
// Input NoData value is meaningless after scaling, so we set the output NoData value to NaN. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BandAffineCombination has an optional
" <Argument name='dst_nodata' type='double' "
"description='Override output nodata value'/>"
151ad77
to
f0ffadc
Compare
Adds an algorithm to unscale rasters before they are input to other algorithms such as expression evaluation.