WIP: 109 layer descriptor shape parameters #110

brian-r-calder · 2024-06-25T14:37:31Z

Issue #109: layers do not currently have separate shape parameters (except Geometadata layer), but should in order for VR refinements to work appropriately. This pull request adds this functionality, and also bug-fixes a number of issues in the VR implementation, and other deprecation issues (e.g., snprintf() rather than sprintf()).

In order to allow for layers that have different dimensions from the main grid (specifically the VRRefinements and VRNode layers), added support for dimensions in the LayerDescriptor class, and propagated support to the derived classes.

VR data in field BAG files appears to contain 2D arrays rather than the 1D version that they should; modified code to follow this model, rather than the standard so that source data can be read. Replaced deprecated sprintf() with snprintf().

WIP debugging to confirm location of memory leak causing heap-bombs elsewhere (commit purely to allow publication and other support activities).

selimnairb · 2024-11-22T19:20:02Z

api/bag_vrrefinements.cpp

+    hsize_t dims[2];
+    int ndims = h5dataSet->getSpace().getSimpleExtentDims(dims, nullptr);
+    if (ndims != 2) {
+        throw InvalidVRRefinementDimensions{};


@brian-r-calder Here we are checking for there to be two dimensions, if there aren't, an InvalidVRRefinementDimensions exception is thrown. However, the message from InvalidVRRefinementDimensions currently reads: "The variable resolution refinement layer is not 1 dimensional.", which is confusing. Can you clarify?

@brian-r-calder Relatedly, test_bag_valuetable.cpp is failing in test case TEST_CASE("test georeferenced metadata layer readVR/writeVR", "[georefMetadatalayer][readvr][writevr]") because the test VR dataset is being used is one-dimensional, which is triggering the above exception. Does the test need to be updated, or the code?

This is a case of adapting to the reality of BAG files in the field. Although the specification says that VR refinements is a one-dimensional array, and should be written into the HDF5 file as a single dimensional dataset, BAG files retrieved from NCEI (from NOAA surveys) do not have this form. Although not entirely proven, I suspect that this is because the GDAL drivers are not following the specification as written.

We have a couple of options here: we can attempt to get the GDAL drivers re-written and then re-write ever BAG file in the US national archive (and presumably everywhere else that BAG files are created using GDAL -- meaning everything that uses CARIS software, most likely); or, we can adapt the reference library to match the field experience. Given the level of complexity in the former, I've chosen to pursue the latter.

To fix this, I've changed the exception to say that the dimensions are "inconsistent with specification", but this also means that we'll have to change the test to reflect this.

@brian-r-calder The check for ndims != 2 causes exception InvalidVRRefinementDimensions when our tests try to read back VR data previously written by the test, see here, which is triggered here. In this case, because the BAG library created the VRRefinements layer, ndim=1.

I think we need to be able to handle either ndims=1 or ndims=2.

I think that might be less useful than making the code write ndims=2 rather than ndims=1 --- the goal with BAG has always been to attempt to have as few options like this as possible so as to keep the code simple. Since we're rather Overtaken By Events here (the archive at NCEI already containing BAGs that have ndims=2 due to incautious vendors not checking the specifications) I'd advocate for switching everything to ndims=2, including the specification.

selimnairb · 2024-11-22T19:47:16Z

api/bag_layer.cpp

    uint32_t numRows = 0, numColumns = 0;
-    std::tie(numRows, numColumns) = pDataset->getDescriptor().getDims();
+    std::tie(numRows, numColumns) = m_pLayerDescriptor->getDims();

    if (columnEnd >= numColumns || rowEnd >= numRows)


@brian-r-calder This branch is being taken when running test_bag_vrmetadata.cpp, specifically when the test attempts to read back the VR metadata. Is the test wrong, or is there a mistake in Layer::read()?

I don't see a branch here, but the code is correct --- we have to read the dimensions specifically from the layer rather than from the dataset, since we need to allow each layer to have its own dimensions.

What I meant was if (columnEnd >= numColumns || rowEnd >= numRows) evaluates to true when Layer::read() is called from test_bag_vrmetadata.cpp:452. Is the VR layer being created by the test incorrect?

When reporting exceptions for VR refinements not having the right number of dimensions, we have both 1D and 2D arrays that have to be handled (the 2D simply because we're patching around a bug in BAG creation in other software that doesn't use this code that's not following the specification correctly). The reporting of InvalidVRRefinementDimensions therefore has to be neutral on number of dimensions and report that the number is inconsistent.

… in addition to XML file on filesystem, to provide diagnostic info when tests crash

… common usage that is counter to the BAG spec (i.e. 1D)

…to read-back dta after it was written to disk in accordance with other tests (such as 'test vr metadata create open' in the same test file)

…C++ header file public interfaces

selimnairb · 2025-01-03T16:08:02Z

api/bag_layerdescriptor.cpp

@@ -30,6 +30,7 @@ LayerDescriptor::LayerDescriptor(
    std::string internalPath,
    std::string name,
    LayerType type,
+    uint32_t rows, uint32_t cols,


@brian-r-calder We are defining rows and cols as uint32_t but these are always set from hsize_t values. hsize_t is a typedef of uint64_t, so an implicit narrowing is taking place. This doesn't seem to be a problem for GCC on Linux, but it fails by default in Visual Studio, and is a potential source of error.

Any objections to making these uint64_t?

No, that should be fine with most modern implementations. It was historically uint32_t because not all systems had moved to first-class uint64_t support at the time, I think.

Okay, I'll make this change.

…than uint32_t like elsewhere in the API) because dimensions are initialized from HDF5 hsize_t values, which are uint64_t.

…ying uint64_t to maintain compatibility with the rest of the BAG API, which assumes rows and cols are uint32_t.

…s other implementations do) rather than 1D (as called for in BAG spec)

selimnairb · 2025-01-04T21:30:03Z

api/bag_vrnode.cpp

@@ -354,7 +367,14 @@ void VRNode::writeProxy(
            throw DatasetNotFound{};

        auto pDataset = this->getDataset().lock();
+        // TODO: Confirm that this is what we want --- this resets the dimensions of the


@brian-r-calder The code in this branch needs to be updated to account for there now being two dimensions in the file data space (see here for example). However, having this modify the main BAG dataset dimensions seems wrong. I'm not sure what the right answer is though.

selimnairb · 2025-01-04T21:30:42Z

api/bag_vrrefinements.cpp

@@ -321,7 +336,14 @@ void VRRefinements::writeProxy(
            throw DatasetNotFound{};

        auto pDataset = this->getDataset().lock();
+        // TODO: Confirm that this is what we want --- this resets the dimensions of the


@brian-r-calder The code in this branch needs to be updated to account for there now being two dimensions in the file data space (see here for example). However, having this modify the main BAG dataset dimensions seems wrong. I'm not sure what the right answer is though.

…ntations do) rather than 1D (as called for in BAG spec)

selimnairb · 2025-01-06T20:19:54Z

api/bag_vrnodedescriptor.cpp

@@ -55,7 +58,7 @@ std::shared_ptr<VRNodeDescriptor> VRNodeDescriptor::create(
    int compressionLevel)
 {
    return std::shared_ptr<VRNodeDescriptor>(
-        new VRNodeDescriptor{dataset.getNextId(), chunkSize,
+        new VRNodeDescriptor{dataset.getNextId(), 1, 1, chunkSize,


@brian-r-calder I changed this to 1 row and 1 column to get the existing tests to pass, but it seems like this should start out as 0, 0 and only be changed if a node is added. What do you think?

selimnairb · 2025-01-06T20:20:25Z

api/bag_vrrefinementsdescriptor.cpp

@@ -54,7 +56,7 @@ std::shared_ptr<VRRefinementsDescriptor> VRRefinementsDescriptor::create(
    int compressionLevel)
 {
    return std::shared_ptr<VRRefinementsDescriptor>(
-        new VRRefinementsDescriptor{dataset.getNextId(), chunkSize,
+        new VRRefinementsDescriptor{dataset.getNextId(), 1, 1, chunkSize,


@brian-r-calder I changed this to 1 row and 1 column to get the existing tests to pass, but it seems like this should start out as 0, 0 and only be changed if a refinement is added. What do you think?

…spaces

…ption handling for HDF5 call in Dataset::readDataset()

…sions

brian-r-calder added 3 commits June 17, 2024 18:36

Added support for layer-specific dimensions

a4f69d6

In order to allow for layers that have different dimensions from the main grid (specifically the VRRefinements and VRNode layers), added support for dimensions in the LayerDescriptor class, and propagated support to the derived classes.

Bug-fixes for VR readers

afa60bc

VR data in field BAG files appears to contain 2D arrays rather than the 1D version that they should; modified code to follow this model, rather than the standard so that source data can be read. Replaced deprecated sprintf() with snprintf().

Update bag_vrrefinements.cpp

ca684a5

WIP debugging to confirm location of memory leak causing heap-bombs elsewhere (commit purely to allow publication and other support activities).

brian-r-calder added bug Something isn't working enhancement New feature or request labels Jun 25, 2024

Adjustment to remove debugging code after initial tests.

78a1205

selimnairb mentioned this pull request Jul 12, 2024

How do you open older .BAG files? #90

Open

selimnairb added 2 commits November 22, 2024 10:08

Merge branch 'master' into 109-LayerDescriptor-shape-parameters

6fd3fb9

Fix tests broken by addressing #109.

b45aecf

selimnairb reviewed Nov 22, 2024

View reviewed changes

selimnairb and others added 9 commits December 6, 2024 10:58

CI: Enable examples building for test reporting workflow

a3cbb27

Fix single-line if statement error; added braces.

c8b6515

Fix single-line if statement error; added braces.

f9e186e

CI: test reporting: Make sure test output is reported to the console,…

7c75ebf

… in addition to XML file on filesystem, to provide diagnostic info when tests crash

Support VR elements stored in either 1D or 2D arrays to be compatible…

2cc6302

… common usage that is counter to the BAG spec (i.e. 1D)

test: Restructure test 'test vr metadata write read' to only attempt …

d49c72d

…to read-back dta after it was written to disk in accordance with other tests (such as 'test vr metadata create open' in the same test file)

SWIG: Python: first pass updating swig .i files to mirror changes to …

3b91e1e

…C++ header file public interfaces

Tests: Python: Update tests to reflect layer dimension API fixes

39cee19

selimnairb reviewed Jan 3, 2025

View reviewed changes

selimnairb added 3 commits January 3, 2025 14:07

Store dimensions of layer descriptors as uint64_t internally (rather …

566324e

…than uint32_t like elsewhere in the API) because dimensions are initialized from HDF5 hsize_t values, which are uint64_t.

LayerDescriptor: Return dimensions as uint32_t rather than the underl…

65c33d2

…ying uint64_t to maintain compatibility with the rest of the BAG API, which assumes rows and cols are uint32_t.

Partial fix to update VR refinements and nodes to use 2D dataspace (a…

98344af

…s other implementations do) rather than 1D (as called for in BAG spec)

selimnairb reviewed Jan 4, 2025

View reviewed changes

selimnairb added 2 commits January 6, 2025 12:17

Merge branch 'master' into 109-LayerDescriptor-shape-parameters

72b166d

Update VR refinements and nodes to use 2D dataspace (as other impleme…

d7cd221

…ntations do) rather than 1D (as called for in BAG spec)

selimnairb reviewed Jan 6, 2025

View reviewed changes

selimnairb added 7 commits January 6, 2025 15:27

Ensure VR descriptors are consistent with shape of underlying H5 data…

27189ec

…spaces

Remove unnecessary casts

60a7a92

Add explicit cast to uint32_t in LayerDescriptor::getDims()

065acbd

Fix wheel build on Windows

728ee2f

VR: Add test_vr_bag to test reading an existing VR BAG file; Add exce…

772e7c9

…ption handling for HDF5 call in Dataset::readDataset()

test_vr_bag: Ensure BAG dimensions match VR metadata descriptor dimen…

27a6687

…sions

tests: Add example VR BAG from NBS archive

cd83fda

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: 109 layer descriptor shape parameters #110

WIP: 109 layer descriptor shape parameters #110

brian-r-calder commented Jun 25, 2024

selimnairb Nov 22, 2024

selimnairb Nov 22, 2024

brian-r-calder Dec 21, 2024

selimnairb Jan 2, 2025

brian-r-calder Jan 3, 2025

selimnairb Nov 22, 2024 •

edited

Loading

brian-r-calder Dec 21, 2024

selimnairb Jan 2, 2025

selimnairb Jan 3, 2025 •

edited

Loading

brian-r-calder Jan 3, 2025

selimnairb Jan 3, 2025

selimnairb Jan 4, 2025

selimnairb Jan 4, 2025

selimnairb Jan 6, 2025

selimnairb Jan 6, 2025

WIP: 109 layer descriptor shape parameters #110

Are you sure you want to change the base?

WIP: 109 layer descriptor shape parameters #110

Conversation

brian-r-calder commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selimnairb Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selimnairb Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selimnairb Nov 22, 2024 •

edited

Loading

selimnairb Jan 3, 2025 •

edited

Loading