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

Forcings Engine Data Provider Base Interface #839

Merged
merged 12 commits into from
Jul 1, 2024

Conversation

program--
Copy link
Contributor

@program-- program-- commented Jun 25, 2024

Important

This PR replaces #720.

This PR adds the ForcingsEngineDataProvider<DataType, SelectionType> class. This class is intended to serve as the base class for Lumped, Gridded, and Unstructured forcings engine data providers. The base class handles timing and updating the underlying BMI instance, so derived classes need only implement the value querying requirements.

Internally, this PR also adds the detail::ForcingsEngineStorage class, which composes a map of initialization file names to Python BMI Adapter shared pointers. The design of this is to allow the Forcings Engine data providers to store a mask of some sort to simplify querying from the forcings engine, i.e. for a Gridded instance, the mask may be a bounding box. This need is due to the implementation of the forcings engine, and should simplify the external requirements for a NGen run using the Python Forcings Engine.

Additionally, this PR includes some minor changes to the forcings library. Namely, the GenericDataProvider class is refactored into a type alias, and the NullForcingProvider implementation is split into header/source files rather than being header-only. The latter change is to ensure the forcings CMake target is always build-able.

Additions

  • ForcingsEngineDataProvider: templated base class.
  • detail::ForcingsEngineStorage: shared storage between data providers.
  • detail::assert_forcings_engine_requirements: helper for ensuring runtime dependencies are available.
  • detail::parse_time: helper for parsing timestamp strings.
  • constexpr constants for default time format string and forcings engine python information.

Changes

  • NullForcingProvider: split implementation into header/source file.
  • GenericDataProvider: converted into type alias.

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

@PhilMiller
Copy link
Contributor

The shadowed variable bmi_ is a substantial concern. The other comments are inconsequential.

@program-- program-- self-assigned this Jun 26, 2024
PhilMiller
PhilMiller previously approved these changes Jun 26, 2024
Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Maybe good to let others have a look, or see what the derived classes look like, but I think this is all good.

@program-- program-- force-pushed the jsm-forcings-engine-provider branch from fd03f30 to b8471cc Compare June 26, 2024 16:50
@PhilMiller
Copy link
Contributor

The main header here can only be included with NGEN_WITH_PYTHON enabled, but I'm not sure that's actually something we need to guard against in the header itself. Implementations and clients are going to need to guard against it anyway. I guess it would save clients conditionalizing both the #include and the usage site.

@PhilMiller PhilMiller force-pushed the jsm-forcings-engine-provider branch from 20ae487 to b01255d Compare June 27, 2024 00:49
donaldwj
donaldwj previously approved these changes Jun 28, 2024
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 dont see any problems

//! Utility function for ForcingsEngineLumpedDataProvider constructor.
time_t parse_time(const std::string& time, const std::string& fmt);

//! Check that requirements for running the forcings engine
Copy link
Contributor

@donaldwj donaldwj Jun 28, 2024

Choose a reason for hiding this comment

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

Since this is probably a fatal error on this function is fine I would still want a check form of the function but it is not important

std::string init_;

//! Calendar time for simulation beginning
clock_type::time_point time_begin_{};
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need an official style rule on member data names (or names in general), we have at least 4 styles used between various files

src/forcing/ForcingsEngineDataProvider.cpp Show resolved Hide resolved
@program-- program-- merged commit c53584a into NOAA-OWP:master Jul 1, 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.

3 participants