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

BAG dataset - calculate the northeast pixel corner rather than scaling the resolution, due to an incorrectly shifted northeast corner in some CARIS surveys #1728

Merged
merged 12 commits into from Jul 19, 2019

Conversation

@zacharyburnettNOAA
Copy link
Contributor

zacharyburnettNOAA commented Jul 17, 2019

What does this PR do?

attempts to fix issue brought up in #1643

What are related issues/pull requests?

#1643

@@ -3346,8 +3345,7 @@ void BAGDataset::LoadMetadata()
{
// Found with https://data.ngdc.noaa.gov/platforms/ocean/nos/coast/H12001-H14000/H12525/BAG/H12525_MB_4m_MLLW_1of2.bag
// https://github.com/OSGeo/gdal/issues/1643
CPLDebug("BAG", "cornerPoints seems to use a PixelIsArea convention.");
bPixelCenterConvention = false;
CPLError(CE_Failure, CPLE_AppDefined,"Ignoring BAG Upper Right Corner.");

This comment has been minimized.

Copy link
@rouault

rouault Jul 17, 2019

Member

I'd wish we maintain the ability to handle in a nice way those datasets, even if there are "illegal" given the specification (buggy products do exist in the wild, and when it is possible and reasonable code-wise to handle them, we should try). I'm OK with emitting a message, but rather a CE_Warning than a CE_Failure, and with a more explicit message saying that the product uses a PixelIsArea convention which is non-standard

This comment has been minimized.

Copy link
@rouault

rouault Jul 17, 2019

Member

In the final else case with the 3 CPLDebug() messages, you shoud also emit a CPLError(CE_Warning, ...) since that latter case is more annoying that the PixelIsArea case: here the georeferencing is clearly inconsistent !

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 18, 2019

Contributor

Our understanding of the meaning of these errors is limited and we are more than happy to address these cases as you suggest. Our only goal here is to ignore the upper right corner rather than scale the resolution. Since BAGs should not be pixeliscorner we wanted to remove that as an option for writing as well.

gdal/frmts/hdf5/bagdataset.cpp Show resolved Hide resolved
data = gdal.VSIFReadL(1, 1000000, f)
gdal.VSIFCloseL(f)
gdal.Unlink('/vsimem/out.bag')
ds = gdal.Open('data/test_offset_ne_corner.bag')

This comment has been minimized.

Copy link
@rouault

rouault Jul 17, 2019

Member

is that one completely inconsistent, or using a pixelisarea convention ? I'd wish we keep a test for a dataset with a pixelisarea convention

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 18, 2019

Contributor

We were attempting to add a test case for BAGs that were being assessed as pixeliscorner due to the upper right corner being offset to ensure they were being handled as intended.

adfGeoTransform[1] = (dfURX - dfLLX) / (m_nLowResWidth - (bPixelCenterConvention ? 1 : 0));
adfGeoTransform[3] = dfURY;
adfGeoTransform[5] = (dfLLY - dfURY) / (m_nLowResHeight - (bPixelCenterConvention ? 1 : 0));
adfGeoTransform[1] = dfResWidth;

This comment has been minimized.

Copy link
@rouault

rouault Jul 17, 2019

Member

shoud be modiied to behave nicely when PixelIsArea convention is detected

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 18, 2019

Contributor

Our understanding of the previous code was that PixelIsCorner was assumed when the upper left corner was not written correctly. This was resulting in the resolution being scaled, which is not what was intended. As was described in #1643, the BAG is defined by the lower lefter corner and the resolution. The upper right corner is only written because of our understanding of ISO 19115, and apparently is being written incorrectly because there is no check on its validity when written by the associated software. We hope to correct this.

This comment has been minimized.

Copy link
@rouault

rouault Jul 18, 2019

Member

Our understanding of the previous code was that PixelIsArea was assumed when the upper left corner was not written correctly. This was resulting in the resolution being scaled, which is not what was intended.

your proposed changes regarding set adfGeoTransform[] look good, except than the half-pixel shift of the origin must not be done in the PixelIsArea/PixelCorner case.

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 18, 2019

Contributor

We did the half shift to the extracted GeoTransform because it was consistent with the rest of the GDAL BAG driver. It looked like the way the driver was written (and all the tests worked) took care of the shift during reading and writing, thus the GDAL BAG object took on a PixelIsCorner convention. Should it not be that way? Should we should modify the GeoTransform (and all the tests) to keep the BAG as PixelIsArea as a GDAL object? There must be an attribute we would set that informs a transform to or from other object type of any conversion that needs to be made to the associated GeoTransform.

This comment has been minimized.

Copy link
@rouault

rouault Jul 18, 2019

Member

We did the half shift to the extracted GeoTransform because it was consistent with the rest of the GDAL BAG driver

That's fine. I meant that when the convention of the product is detected to be the non-standard/erroneous PixelCorner/PixelIsArea then this half-pixel shift is not needed, since it is already in the georeferencing convention used by GDAL.

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 18, 2019

Contributor

From our perspective the non-standard detection indicates that the upper right corner was written incorrectly and thus a warning should be thrown. It does not mean the BAG is PixelIsCorner. Since the code (now) always recalculates the upper right corner there is no need to make the half pixel shift conditional. The resolution and lower left corner are always correct and the raster is always PixelIsArea.

This comment has been minimized.

Copy link
@rouault

rouault Jul 18, 2019

Member

It does not mean the BAG is PixelIsCorner

Hum, perhaps... Is there a way of checking what the correct georeferencing would be for a product like
https://data.ngdc.noaa.gov/platforms/ocean/nos/coast/H12001-H14000/H12525/BAG/H12525_MB_4m_MLLW_1of2.bag which was referenced in #1643 ? If indeed this is just an issue with the computation of the upper-right corner, then d455285 should be reverted. This is becoming confusing... :-)

This comment has been minimized.

Copy link
@GlenRice-NOAA

GlenRice-NOAA Jul 22, 2019

Contributor

We went back and compared the source raster with the derived BAG for a few of our products and found the lower left corner and resolution were correct. The upper right corner was reported as being one grid resolution away from what is reported in the BAG, so clearly it was being ignored by the software. We think this confirms the steps taken here.
We often say these things are not hard, but there are confusing. Thanks for accepting the work.

This reverts commit d455285.
Copy link
Member

rouault left a comment

ok, I think we're good now. Anything else on your side ?

@zacharyburnettNOAA

This comment has been minimized.

Copy link
Contributor Author

zacharyburnettNOAA commented Jul 19, 2019

No, I don't think there are any more issues. Thank you for reviewing the changes.

@rouault rouault merged commit 9f1bfb8 into OSGeo:master Jul 19, 2019
3 checks passed
3 checks passed
OSGeo.gdal.doc Build #20190718.7 succeeded
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
zacharyburnettNOAA added a commit to GlenRice-NOAA/gdal that referenced this pull request Jul 19, 2019
@zacharyburnettNOAA zacharyburnettNOAA mentioned this pull request Jul 23, 2019
5 of 5 tasks complete
rouault added a commit that referenced this pull request Jul 23, 2019
…olution, due to an incorrectly shifted northeast corner in some CARIS surveys (#1728)
rouault added a commit that referenced this pull request Jul 23, 2019
…olution, due to an incorrectly shifted northeast corner in some CARIS surveys (#1728)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.