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

GeoPackage input support #522

Merged
merged 62 commits into from
Jul 18, 2023
Merged

GeoPackage input support #522

merged 62 commits into from
Jul 18, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented Apr 26, 2023

This PR implements support for GeoPackage input via the SQLite3 C library based on the OGC GeoPackage Encoding Standard.

(resolves #295; related: #496)

Note: the current additions are a bit of a mess, but will be cleaned up once this PR is no longer a draft.

Additions

  • Adds SQLite3 as a dependency.
  • Adds a simple C++ wrapper over the SQLite3 C interface (include/geopackage/SQLite.hpp).
  • Adds a new sub-library geopackage that contains the primary function geopackage::read(), which returns a geojson::GeoJSON (a.k.a. std::shared_ptr<geojson::FeatureCollection>) object, like geojson::read.
  • Adds a WKB reader (geopackage::wkb). [ref: OGC SFA Part 1, GEOS WKB]

Testing

  • gcc (version 13.1.1)
  • clang (version 15.0.7)
  • icx (version 2023.0.0.20221201)

Notes

  • This implementation currently depends on geojson for feature classes.
  • In the future (out of scope for this PR), it may be beneficial to decouple the feature classes implementation from the GeoJSON parser, since the feature classes themselves do not require use of boost::property_tree, and so that this implementation doesn't require linking to geojson.
  • If GDAL is supported in the future, this implementation can be easily refactored -- and modified to be generic for any of the GDAL/OGR vector drivers.
  • Once implemented, lines 262 and 266 should be able to be modified for use with either GeoJSON or GeoPackage, since they will return the same type of object:

    ngen/src/NGen.cpp

    Lines 262 to 266 in 6ab044e

    geojson::GeoJSON nexus_collection = geojson::read(nexusDataFile, nexus_subset_ids);
    std::cout << "Building Catchment collection" << std::endl;
    // TODO: Instead of iterating through a collection of FeatureBase objects mapping to catchments, we instead want to iterate through HY_Catchment objects
    geojson::GeoJSON catchment_collection = geojson::read(catchmentDataFile, catchment_subset_ids);

Todos

  • geopackage::read
    • geopackage::build_feature
    • geopackage::build_geometry
    • geopackage::build_properties
  • Implement Standard WKB reader
    • Handle projections (i.e. Hydrofabric data typically is in EPSG:5070)
    • Extend reader for GeoPackage blob WKB
  • Add SQLite dependency within cmake
  • Unit tests
    • SQLite Wrapper
    • GeoPackage
    • WKB Reader
  • Integration tests (i.e. successfully use geopackage::read in place of the geojson variant)
    • Add a geopackage integration test to the existing github actions workflows
  • Documentation
  • Make GeoPackage support conditional on if SQLite is found. Otherwise, fall back to GeoJSON.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux

@program--
Copy link
Contributor Author

This PR is just about ready for review. The last things are adding unit tests for geopackage::read and it's used functions, as well as run the ngen integration tests using geopackage::read instead of geojson::read. I'm expecting the need to refactor quite a bit of the work I've done here (especially the WKB reader), since it is messy 😅.

@program--
Copy link
Contributor Author

program-- commented May 9, 2023

There is currently an issue in this PR that occurs when the CMake Build type is set to Release with GCC and Clang. For some reason, the boost::geometry::srs::transformation<> object throws an error when its constructor is called.

This does not occur when The build type is set to Debug, OR when ICX is used in either Debug or Release.


After some investigating, it seems like it might be a clang or boost bug? Compiling geometry.cpp to LLVM IR with -O1 results in recursive calls to llvm::ScalarEvolution::getRangeReg (which I'm guessing results in a stack overflow, hence the segfault?):

#12 0x00007fffefc65393 llvm::ScalarEvolution::getRangeRef(llvm::SCEV const*, llvm::ScalarEvolution::RangeSignHint) (/usr/lib/libLLVM-15.so+0x2866e98)
... repeats ...
#255 0x00007f9404c66e98 llvm::ScalarEvolution::getRangeRef(llvm::SCEV const*, llvm::ScalarEvolution::RangeSignHint) (/usr/lib/libLLVM-15.so+0x2866e98)

and emitting the .ll file, before optimization, is almost 1M lines...

EDIT: this also occurs with clang 14.0.6 so it doesn't seem like a recent regression


EDIT: after talking with Donald, working around this by disabling optimizations for geometry.cpp when building with gcc/clang may be the way to go. Commit c6b31e4 handles this.

Copy link
Contributor

@mattw-nws mattw-nws left a comment

Choose a reason for hiding this comment

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

Will look at this more, but did a cursory pass and don't see any show stoppers. Interested in others' feedback. I did run the software on the test case in data/gauge_01073000 using both the geojson and geopkg inputs, which succeeded and I found no differences in the output, so that's positive.

src/NGen.cpp Show resolved Hide resolved
Copy link
Member

@hellkite500 hellkite500 left a comment

Choose a reason for hiding this comment

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

Generally looks pretty good, but a couple things that might warrant further discussion/testing, and some minor cleanup comments.

include/geopackage/SQLite.hpp Outdated Show resolved Hide resolved
include/geopackage/SQLite.hpp Outdated Show resolved Hide resolved
include/geopackage/WKB.hpp Outdated Show resolved Hide resolved
src/geopackage/CMakeLists.txt Outdated Show resolved Hide resolved
src/geopackage/feature.cpp Show resolved Hide resolved
src/geopackage/geometry.cpp Outdated Show resolved Hide resolved
src/geopackage/properties.cpp Outdated Show resolved Hide resolved
src/geopackage/read.cpp Show resolved Hide resolved
src/geopackage/read.cpp Show resolved Hide resolved
@PhilMiller
Copy link
Contributor

Would it make sense to move all of the added SQLite support code over to utilities, since nothing in it is actually specified to GeoPackage?

program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
Review changes per NOAA-OWP#522 (comment).
This commit also reorganizes the WKB implementation by moving inline
definitions to a source file, since there is no need to inline those
functions.
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
@program--
Copy link
Contributor Author

program-- commented Jul 10, 2023

Would it make sense to move all of the added SQLite support code over to utilities, since nothing in it is actually specified to GeoPackage?

@PhilMiller Maybe? Although, GeoPackage is the only portion of NGen that is using SQLite, so in that case it would also make sense to keep it together with the GeoPackage code. I think it would make sense to move it if other portions of NGen made use of SQLite.

program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
program-- added a commit to program--/ngen that referenced this pull request Jul 10, 2023
Copy constructor/assignment operator are not used for the `sqlite`
class, and should subsequently be deleted. This change is implemented
per NOAA-OWP#522 (comment).
@mattw-nws mattw-nws requested a review from hellkite500 July 17, 2023 16:58
@mattw-nws mattw-nws merged commit d8ca23f into NOAA-OWP:master Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
Review changes per #522 (comment).
This commit also reorganizes the WKB implementation by moving inline
definitions to a source file, since there is no need to inline those
functions.
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
Copy constructor/assignment operator are not used for the `sqlite`
class, and should subsequently be deleted. This change is implemented
per #522 (comment).
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
changes per #522 (comment)

also modifies switch to return instead of assign to `g`.
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
changes per #522 (comment)
and also modifies `std::make_shared` calls within the switch statement
to reflect forwarding, instead of construction before passing.
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
mattw-nws pushed a commit that referenced this pull request Jul 18, 2023
@program-- program-- deleted the geopackage branch May 29, 2024 16:03
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.

GeoPkg Input Support
4 participants