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.

Sign up for GitHub

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

Jump to bottom

Gridded Forcings Engine Data Provider #828

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Gridded Forcings Engine Data Provider #828

wants to merge 6 commits into from

Conversation

program--
Copy link
Contributor

@program-- program-- commented May 31, 2024 •
edited
Loading

This PR closes #705, and partially depends on #845 (CMake changes, test files).

Adds a ForcingsEngineGriddedDataProvider class inheriting from ForcingsEngineDataProvider<double, GriddedDataSelector>.

This provider provides the same interface as #845 but for gridded forcings, i.e. GRID_TYPE="gridded" in the forcings engine configuration file.

Similar to how the lumped provider associates with a divide ID, the gridded provider associates with a mask of the underlying hydrofabric domain using a minimum bounding rectangle around a divide:

where,

  • C1 - C5: Hydrofabric domain
  • A/B: Minimum/maximum corners of hydrofabric grid
  • C/D: Minimum/maximum corners of masked grid

Note

Related structs/functions for grid masking and indexing are:

  • data_access::GridSpecification (Provider header)
  • data_access::GridMask (Provider header)
  • construct_grid_spec(...) (Provider source)
  • construct_grid_mask(...) (Provider source)

Each provider assigned to a catchment will only provide data from the hydrofabric grid for their masked grid.

Additions

  • Adds ForcingsEngineGriddedDataProvider class.
  • Adds geojson::box_t representing a geographic bounding box type.

Changes

  • GridDataSelector -> GriddedDataSelector

Todos

  • Clean up GridDataSelector simplification & remove deprecated test.
  • Integrate tests with files from Lumped Forcings Engine Data Provider #845.
  • Investigate forcings engine exceptions when using gridded GRID_TYPE. Particularly, broadcasting and (legacy?) indexing schemes in geoMod.py (Includes file size optimization for domain file)
  • Rethink MPI finalization in unit tests
  • Investigate ESMF -> NetCDF -> gfortran segfault (see below, might be a local issue)

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 ➡️
Segfault Details
AddressSanitizer:DEADLYSIGNAL
=================================================================
==1730553==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x740a134d5e04 bp 0x508000031e20 sp 0x7fffdaeda648 T0)
==1730553==The signal is caused by a READ memory access.
==1730553==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x740a134d5e04 in _gfortran_string_len_trim /usr/src/debug/gcc/gcc/libgfortran/intrinsics/string_intrinsics_inc.c:231:7
    #1 0x740a134d5e04 in _gfortran_string_len_trim /usr/src/debug/gcc/gcc/libgfortran/intrinsics/string_intrinsics_inc.c:186:1
    #2 0x7409c1639be8 in __netcdf_nc_interfaces_MOD_addcnullchar (/usr/lib/libnetcdff.so.7+0x10be8) (BuildId: 4572587428b4ce31dcd6cedadb14725729de5f80)
    #3 0x7409c163b45b in nf_open_ (/usr/lib/libnetcdff.so.7+0x1245b) (BuildId: 4572587428b4ce31dcd6cedadb14725729de5f80)
    #4 0x7409c16e53d6 in __netcdf_MOD_nf90_open (/usr/lib/libnetcdff.so.7+0xbc3d6) (BuildId: 4572587428b4ce31dcd6cedadb14725729de5f80)
    #5 0x74096a3d2735 in __esmf_factorreadmod_MOD_esmf_factorread (/usr/local/lib/libO/Linux.gfortran.64.intelmpi.default/libesmf_fullylinked.so+0xbd2735) (BuildId: 432f311ba11cf85b40b29d5c224091c8ea93a341)
    #6 0x74096a60c22c in __esmf_fieldsmmmod_MOD_esmf_fieldsmmstorefromfile (/usr/local/lib/libO/Linux.gfortran.64.intelmpi.default/libesmf_fullylinked.so+0xe0c22c) (BuildId: 432f311ba11cf85b40b29d5c224091c8ea93a341)
    #7 0x74096a41c1d6 in f_esmf_smmstore_ (/usr/local/lib/libO/Linux.gfortran.64.intelmpi.default/libesmf_fullylinked.so+0xc1c1d6) (BuildId: 432f311ba11cf85b40b29d5c224091c8ea93a341)
    #8 0x740969db1f1a in ESMCI::Field::smmstore(ESMCI::Field*, ESMCI::Field*, char const*, ESMCI::RouteHandle**, ESMC_Logical*, int*, int*) (/usr/local/lib/libO/Linux.gfortran.64.intelmpi.default/libesmf_fullylinked.so+0x5b1f1a) (BuildId: 432f311ba11cf85b40b29d5c224091c8ea93a341)
    #9 0x740969b71e5c in ESMC_FieldSMMStore (/usr/local/lib/libO/Linux.gfortran.64.intelmpi.default/libesmf_fullylinked.so+0x371e5c) (BuildId: 432f311ba11cf85b40b29d5c224091c8ea93a341)
    #10 0x740a1319b595 in ffi_call_unix64 /usr/src/debug/libffi/libffi-3.4.6/x86_64-pc-linux-gnu/../src/x86/unix64.S:104
    #11 0x740a1319800d in ffi_call_int /usr/src/debug/libffi/libffi-3.4.6/x86_64-pc-linux-gnu/../src/x86/ffi64.c:673:3
    #12 0x740a1319abd2 in ffi_call /usr/src/debug/libffi/libffi-3.4.6/x86_64-pc-linux-gnu/../src/x86/ffi64.c:710:3
    #13 0x740a02fb3be3  (/usr/lib/python3.12/lib-dynload/_ctypes.cpython-312-x86_64-linux-gnu.so+0x6be3) (BuildId: 02904d53ddf37cf007b7cb047417a7d760e541b1)
    #14 0x740a02fbde01  (/usr/lib/python3.12/lib-dynload/_ctypes.cpython-312-x86_64-linux-gnu.so+0x10e01) (BuildId: 02904d53ddf37cf007b7cb047417a7d760e541b1)
    #15 0x740a13f80aba in _PyObject_MakeTpCall (/usr/lib/libpython3.12.so.1.0+0x180aba) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #16 0x740a13f8931e in _PyEval_EvalFrameDefault (/usr/lib/libpython3.12.so.1.0+0x18931e) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #17 0x740a13f83845 in _PyObject_FastCallDictTstate (/usr/lib/libpython3.12.so.1.0+0x183845) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #18 0x740a13fbfaf6  (/usr/lib/libpython3.12.so.1.0+0x1bfaf6) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #19 0x740a13f80a67 in _PyObject_MakeTpCall (/usr/lib/libpython3.12.so.1.0+0x180a67) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #20 0x740a13f8931e in _PyEval_EvalFrameDefault (/usr/lib/libpython3.12.so.1.0+0x18931e) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #21 0x740a13fe166b  (/usr/lib/libpython3.12.so.1.0+0x1e166b) (BuildId: c5b067a238514e950597e6a877ad8cb8b948f6de)
    #22 0x5bf89ccdf0b3 in pybind11::detail::simple_collector<(pybind11::return_value_policy)1>::call(_object*) const /mnt/ssd/work/NOAA/ngen-devel/extern/pybind11/include/pybind11/detail/../cast.h:1465:28
    #23 0x5bf89ccdf0b3 in pybind11::object pybind11::detail::object_api>::operator()<(pybind11::return_value_policy)1, double&>(double&) const /mnt/ssd/work/NOAA/ngen-devel/extern/pybind11/include/pybind11/detail/../cast.h:1635:75
    #24 0x5bf89ccd1b5d in models::bmi::Bmi_Py_Adapter::UpdateUntil(double) /mnt/ssd/work/NOAA/ngen-devel/src/bmi/Bmi_Py_Adapter.cpp:222:5
    #25 0x5bf89ccc079e in data_access::ForcingsEngineGriddedDataProvider::get_values(GriddedDataSelector const&, data_access::ReSampleMethod) /mnt/ssd/work/NOAA/ngen-devel/src/forcing/ForcingsEngineGriddedDataProvider.cpp:117:76
    #26 0x5bf89cc070d8 in ForcingsEngineGriddedDataProviderTest_VariableAccess_Test::TestBody() /mnt/ssd/work/NOAA/ngen-devel/test/forcing/ForcingsEngineGriddedDataProvider_Test.cpp:102:32
    #27 0x5bf89cc98aad in void testing::internal::HandleSehExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)(), char const*) /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2599:10
    #28 0x5bf89cc98aad in void testing::internal::HandleExceptionsInMethodIfSupported(testing::Test*, void (testing::Test::*)(), char const*) /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2635:14
    #29 0x5bf89cc53680 in testing::Test::Run() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2674:5
    #30 0x5bf89cc55f39 in testing::TestInfo::Run() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2853:11
    #31 0x5bf89cc57c91 in testing::TestSuite::Run() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:3012:30
    #32 0x5bf89cc81f67 in testing::internal::UnitTestImpl::RunAllTests() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:5870:44
    #33 0x5bf89cc9affd in bool testing::internal::HandleSehExceptionsInMethodIfSupported(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2599:10
    #34 0x5bf89cc9affd in bool testing::internal::HandleExceptionsInMethodIfSupported(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:2635:14
    #35 0x5bf89cc81200 in testing::UnitTest::Run() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest.cc:5444:10
    #36 0x5bf89ccb67e2 in RUN_ALL_TESTS() /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/include/gtest/gtest.h:2293:73
    #37 0x5bf89ccb67e2 in main /mnt/ssd/work/NOAA/ngen-devel/test/googletest/googletest/src/gtest_main.cc:51:10
    #38 0x740a16834e07 in __libc_start_call_main /usr/src/debug/glibc/glibc/csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #39 0x740a16834ecb in __libc_start_main /usr/src/debug/glibc/glibc/csu/../csu/libc-start.c:360:3
    #40 0x5bf89ca84894 in _start (/mnt/ssd/work/NOAA/ngen-devel/build/test/test_forcings_engine+0x103894) (BuildId: 18db67d88630c25521e1f7e41ac7b7cf536ed86e)

AddressSanitizer can not provide additional info.

SUMMARY: AddressSanitizer: SEGV /usr/src/debug/gcc/gcc/libgfortran/intrinsics/string_intrinsics_inc.c:231:7 in _gfortran_string_len_trim

==1730553==ABORTING

@program-- program-- self-assigned this May 31, 2024
program--
program-- commented Aug 19, 2024
View reviewed changes
data/forcing/forcings-engine/Gauge_01073000_Grid.nc Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I'd like to reduce the size of this -- the majority of the size comes from including the elevation, slope, and aspect for the domain, which currently seems to be a requirement in the forcings engine.

though, documentation states otherwise... need to resolve this still, since this should only be necessary for regridding, but an exception is raised when no regridding is specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible/reasonable to crop it to a smaller spatial domain, that still extends beyond the configured requested area? Or is it already as small as it gets in that respect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's currently cropped to the (3 I think?) divides for the gage in the name -- for strictly testing purposes, I could crop it down to just 1 of the divides... Despite that, the larger issue is on the forcings engine side, so if I can find out more info on what's happening there, I can potentially just get rid of unnecessary variables in the NetCDF file

PhilMiller
PhilMiller reviewed Sep 9, 2024
View reviewed changes
test/forcing/ForcingsEngineGriddedDataProvider_Test.cpp
);

#if NGEN_WITH_MPI
auto comm = MPI_Comm_c2f(MPI_COMM_WORLD);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be explicitly typed for clarity that we care about the specific type

Suggested change
auto comm = MPI_Comm_c2f(MPI_COMM_WORLD);
MPI_Fint comm = MPI_Comm_c2f(MPI_COMM_WORLD);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilMiller PhilMiller PhilMiller left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@program-- program--

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forcing Engine gridded forcing into ngen
2 participants
@program-- @PhilMiller