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

Lumped Forcings Engine Data Provider #845

Merged

Conversation

program--
Copy link
Contributor

@program-- program-- commented Jul 1, 2024

This PR adds the ForcingsEngineLumpedDataProvider class, inheriting from ForcingsEngineDataProvider<double, CatchmentAggrDataSelector>.

This provider interacts with the python-based Forcings Engine when running in "hydrofabric" mode (i.e. in the configuration file, GRID_TYPE="hydrofabric"). The following variables are accessible through the data provider interface:

BMI Variable Alias Description Units
U2D_ELEMENT U2D 10-m U-component of wind m/s
V2D_ELEMENT V2D 10-m V-component of wind m/s
T2D_ELEMENT T2D 2-m Air Temperature K
Q2D_ELEMENT Q2D 2-m Specific Humidity kg/kg
LWDOWN_ELEMENT LWDOWN Surface downward long-wave radiation flux W/m^2
SWDOWN_ELEMENT SWDOWN Surface downward short-wave radiation flux W/m^2
PSFC_ELEMENT PSFC Surface Pressure Pa
RAINRATE_ELEMENT RAINRATE Surface Precipitation Rate mm/s

Within the engine's catchment domain, each variable has an aggregated scalar value for each catchment. As such, every constructed lumped data provider is associated with a single catchment (but, the underlying forcings engine instance is shared between them).

This PR also includes a minor change to the parent base class, which moves the time and config file path member variables into the protected scope to allow printing of them in derived functions/exceptions and prevent the need to perform some conversions within std::chrono if they were kept in the private scope.

These changes were incorporated in #851.

Additions

  • Adds data_access::ForcingsEngineLumpedDataProvider
  • Adds ForcingsEngineLumpedDataProviderTest test fixture using AORC data from AWS S3.
  • Includes a ESMF-compliant mesh for a hydrofabric subset along USGS Gage-01073000.

Testing

  • Unit tests using GCC 13 and Clang 17.

Notes

In order to run the forcings engine unit tests prerequisites must be met:

  • Set the $WGRIB2 environment variable with the path to your wgrib2 executable, e.g.
    export WGRIB2=$(which wgrib2)
    
  • Ensure ESMF and esmpy are installing either system-wide or in your virtual environment.
  • The master branch of https://github.com/NOAA-OWP/ngen-forcing must be installed.

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 ➡️

@program-- program-- force-pushed the jsm-lumped-forcings-engine-provider branch from 5dfb5c4 to b025920 Compare July 29, 2024 16:45
@program-- program-- marked this pull request as ready for review August 8, 2024 18:31
using TestFixture = ForcingsEngineLumpedDataProviderTest;

constexpr const char* TestFixture::config_file;
const std::time_t TestFixture::time_start = data_access::detail::parse_time("2024-01-17 01:00:00");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One note that I just realized I need to change: time_start has no actual impact on the Forcings Engine's time domain, so while this is currently incorrect, the instance still starts at 2023-01-17 01:00:00. From what I can think of currently, there isn't an easy way to assert that the time matches, since only the time duration is exposed from the forcings engine (i.e. get_data_start_time and others report in seconds with an epoch of 0).

This relates back to #858.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jduckerOWP For the Forcings Engine in particular, could it expose a start_time variable via BMI that we can use to cross-check that the model engine's simulation start time matches the forcings engine's configured/available-data start time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternately, could it expose that as a pseudo-variable that we can set via BMI, rather than living in the config file?

Choose a reason for hiding this comment

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

@PhilMiller we certainly can expose a start time via BMI for cross-checking timestamps between the Forcings Engine and the framework. Regarding @program-- comment for the timestamp discrepancy we can discuss this more in our meeting today but generally the NWM operational forecast iterations are initialize at some start time (0z,6z,12z,18z etc.) and the first available forecast data is at forecast hour 1 of the simulation. Therefore, generally hour 0 (or start time of simulation) will basically need to be forced with some "hot start" fields if models require forcings at the actual start time. For retrospective datasets however, we can modify the Forcings Engine to directly provide meteorological forcings at that start time hour we'll just need some revisions to the workflow to make that work.

@PhilMiller
Copy link
Contributor

Other than the divide_idx_ remarks, the rest of this looks pretty good.

"T2D_ELEMENT",
"Q2D_ELEMENT",
"PSFC_ELEMENT",
"RAINRATE_ELEMENT"

Choose a reason for hiding this comment

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

With the recent upgrades pushed to the NextGen Forcings Engine, some operational configurations (HRRR, RAP, GFS, NBM) can now also extract and advertise a new BMI field called "LQFRAC_ELEMENT" (and "LQFRAC_NODE" if unstructured mesh domain configuration). This field is described as the "liquid fraction of precipitation" and has units of % (0-100 range). For this first iteration of the NextGen Forcings Engine, it'll be okay if we leave it out for this PR and we can just merge the data later on if desired. The complexity of that variable field is that at times, it will or won't be available pending on the NWM operational configuration we apply. Will be happy to discuss these details further in our meeting!

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it won't always be present, we shouldn't require it in the test.

Copy link
Contributor

@donaldwj donaldwj left a comment

Choose a reason for hiding this comment

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

I requested one small change but it is not important. Otherwise I see no issues

: &divide_id[separator + 1]
);

return std::atol(split);
Copy link
Contributor

Choose a reason for hiding this comment

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

I normally prefer strtol() to atol(). For most linux compiliers the actual conversion will be a call internal call to strtol so it is easier for strtol calls to be implemented.

Copy link
Contributor Author

@program-- program-- Aug 14, 2024

Choose a reason for hiding this comment

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

Oh this is a good point. I'll make an issue as a reminder to change this. Thanks 😄

@program-- program-- merged commit 761d910 into NOAA-OWP:master Aug 14, 2024
19 checks passed
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.

4 participants