Mdeshotel ngwpc 9713 wrf geo hydro meta

[mdeshotel]

First PR for refactoring GeoMetaWrfHydro. This PR splits GeoMetaWrfHydro into a base class with three children classes for "gridded", "hydrofabric", and "unstructured" grid types.

Additions

Removals

Changes

-This PR splits GeoMetaWrfHydro into a base class with three children classes for "gridded", "hydrofabric", and "unstructured" grid types.
-Update imports
-Update GeoMeta initiation code in bmi_model.py.

Testing

1. Tested RTE tests. c

Screenshots

Notes

Todos

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
• Visually tested in supported browsers and devices (see checklist below 👇)
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

• Linux

[mxkpp]

This is a good start to the restructuring. Please see minor requested changes.

I compared diffs of the various business logic blocks that have been moved into the new inherited classes -- they look behaviorally equivalent to before.

For the hydrofabric case only, I tested by building and running a calibration realization after clearing my existing scratch dir.

[mxkpp]

Some attrs were replaced with properties (like nx_local) but others have a new property and still are an attr as well, like nx_local_elem. Is the intent for all properties to not have a corresponding attr?

[mdeshotel]

The intent was to replace the attributes with properties. All 3 classes do not have a one to one match of attributes/properties. If a child class doesn't have one of the properties then is should be set as an attribute=None in the init. This was not done correctly when you reviewed but should now be corrected. Here is the original code for reference.

In summary:

• gridded: 6 properties; 0 attributes
• unstructured: 4 properties; 2 attributes=none
• hydrofabric: 2 properties; 4 attributes=None

[mxkpp]

Calls to calc_slope, calc_slope_unstructured, and calc_slope_gridded are malformed

[mdeshotel]

This has been fixed.

[mxkpp]

The most recent commit removed esmf_nc = netCDF4.Dataset(self.config_options.geogrid, "r") from the top of calc_slope_gridded. Regardless of the var name, the original code was passing that var into this function and then replacing the object immediately with a read from disk. I think that behavior should be preserved, namely the step of reading from disk at the top of the function. I am not sure why the original code passes a variable in just to immediately replace it, but that aspect should probably be preserved too, for now.

[mxkpp]

Same comment for other instances where the read netCDF4.Dataset(self.config_options.geogrid, "r") was recently removed, those should be reinstated.