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

Pansharpening: require geotransform on panchromatic and multispectral bands #7373

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

rouault
Copy link
Member

@rouault rouault commented Mar 7, 2023

Pansharpening now requires that panchromatic and multispectral bands have valid geotransform (in early versions, it was assumed in the case of missing geotransform that they covered the same geospatial extent). The undocumented VRT pansharpened MSShiftX and MSShiftY options (and the corresponding C++ GDALPansharpenOptions::dfMSShiftX and dfMSShiftY members) have been removed, due to using the inverted convention as one would expect, and being sub-par solution compared to using geotransform to correlate pixels of panchromatic and multispectral bands.

… bands

Pansharpening now requires that panchromatic and multispectral bands have
valid geotransform (in early versions, it was assumed in the case of missing
geotransform that they covered the same geospatial extent).
The undocumented VRT pansharpened MSShiftX and MSShiftY options (and the
corresponding C++ GDALPansharpenOptions::dfMSShiftX and dfMSShiftY members)
have been removed, due to using the inverted convention as one would expect,
and being sub-par solution compared to using geotransform to correlate pixels
of panchromatic and multispectral bands.
@rouault
Copy link
Member Author

rouault commented Mar 8, 2023

CC @tbonfort

Comment on lines +29 to +30
valid geotransform (in early versions, it was assumed in the case of missing
geotransform that they covered the same geospatial extent).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
valid geotransform (in early versions, it was assumed in the case of missing
geotransform that they covered the same geospatial extent).
valid geotransform (in early versions the geotransform was not considered, i.e. it was expected that all bands covered the same geospatial extent).

Copy link
Member Author

Choose a reason for hiding this comment

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

your suggestion is actually not quite true. If the geotransforms of the pan and ms band differed by more than one pixel of the pan, then the SpatialExtentAdjustment=Union default logic would add the necessary padding so their geotransform was taken into account (but with integer rounding, so not a sub-pixel accuracy from what I can see)

@rouault
Copy link
Member Author

rouault commented Mar 15, 2023

@tbonfort Did you get or will you have a chance to test that before I merge?

@tbonfort
Copy link
Member

tbonfort commented Mar 16, 2023

Hi @rouault ,

I just ran a test and I believe there is an issue: I'm getting some ERROR 1: Recursion detected when forcing an offsetted geotransform on the ms dataset (they actually only seem to be warnings as I'm getting some seemingly correct output at the end).

This happens when I am using a vrt on the MS file to force an "incorrect" geotransform for testing.

  • using pan.tif with gt(0,1,0,0,0,1) and ms.tif with gt(0,4,0,0,0,4) (i.e. the nominal case), everything works correctly.

  • If I now create an msbad.vrt file with gt(-100,5,0,-100,0,5) and reference this msbad.vrt instead of ms.tif inside my pansharpening vrt, I get the recursion error. From some quick testing, setting any geotransform different than 0,4,0,0,0,4 raises the error.

  • If I create and use an ms.vrt file that has the "correct" geotransform, there is no error raised.

@tbonfort
Copy link
Member

@rouault
Copy link
Member Author

rouault commented Mar 16, 2023

I just ran a test and I believe there is an issue: I'm getting some ERROR 1: Recursion detected

fixed by commits added to this PR

@tbonfort
Copy link
Member

LGTM. I'll test more extensively in the coming weeks, but I'd say it's ok to merge. thanks @rouault

@rouault rouault added this to the 3.7.0 milestone Mar 16, 2023
@rouault rouault merged commit 6c089d5 into OSGeo:master Mar 16, 2023
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.

None yet

2 participants