All model libs build fine, but ngen build fails on runner macos-latest, but not on linux. See test_surfacebmi_plus_cfe job summary.

Also - same job fails for cfe ngen integration tests.

Will need to update workflow (check changes in cmake flags?)

Hi,

I am interested to test different module options of Noah-MP under NextGen Framework. I tried to change some options in the namelist but it is not reflected in the NextGen Framework output.

Please let me know how can I control the CFE+NoahMP configuration using name list options. Also, I do not see any spin-up options and I would also like to output different NoahMP variables in NextGen output.

Thanks

For testing, we hard-coded these values, but this will get superseded by NetCDF read functionalities in the gridded driver.

I noticed nwm 3.0.6 appears to have added an IMPERV_OPTION to the NoahLSM version being used. Im not familiar with these changes, but I was curious if there are plans to mirror the new functionality in noah-owp-modular?

See the IMPERV_OPTION under the NOAHLSM_OFFLINE section of in the namelist.hrldas file:

IMPERV_OPTION                     = 2  !(0->none; 1->total; 2->Alley&Veenhuis; 9->orig)

Noah-owp will be used in several modes -- e.g., embedded in the nextgen framework, for which minimal output is desired, or in standalone mode by developers, who would typically want to see some runtime output.

We can create options for this in 2 main ways -- one to add a command line argument such as '-v[erbose] [Level]', and another is to make it an option in the namelist. Either can be backward compatible, so that if they're not specified the default behavior is minimal output.

Our current BMI implementation does not include the get_value_ptr functions because no variables have the TARGET attribute. There is a workaround for this where we can apply the TARGET attribute to the BMI object.

From @peckhams and @zhengtaocui:

The target attribute applied to an instance of a class or derived type (i.e., BMI object "this") propagates down to its members, so we can later get C pointers to them with C_LOC. If an object does not have the TARGET attribute or has not been allocated (using an ALLOCATE statement), no part of it can be accessed by a pointer.

Example here: https://github.com/zhengtaocui/bmi-fortran

For this issue, the following files need to be modified to implement the get_value_ptr* functions:

• bmi/bmi.f90
• bmi/bmi_noahowp.f90

Subsequent work to be outlined in another issue should implement the get_value_at_indices* and set_value_at_indices* functions. Once the full set of functions has been implemented, the model engine will need to be updated (@mattw-nws). Additionally, we need to update the unit testing to account for the newly implemented functions.

depending how extensive this need is, it may be useful to collect variable transformations now housed in the noah-mp drivers into a specific routine.

related, from keith: "The current monolith code reads the parameter tables in subroutines at the end called read_mp_veg_parameters , read_mp_soil_parameters, etc. I am in favor of adding these as modules after we modularize the rest of the important bits."

Confirm parameters have correct spatial discretization in the noah_om_grid branch.

https://github.com/NOAA-OWP/noah-owp-modular/blob/noah_om_grid/src/ParametersGridType.f90

Noah-OWP-Modular has some constant variables that go through the same transfer process as the time-/space-varying variables. Test if this still needs to be done. If not, remove variables from the transfer routines that occur each timestep.

Example:

Related to #5

Just noticed that the workflow here is using the ngen build-submodule action to build the NOM library to test the integration with. This will cause the runner to use the pinned ngen submodule commit to build form, not the current code. This likely isn't the intended behavior here.

Can either toy with the ngen submod build action to try to get the correct code/pr-branch updated in the runner's ngen clone, or simply build the ngen lib from source (would be easier once #103 is complete). Could also hack somewhere in the middle and use the checked out code to overwrite the dir and try to re-build via a second action step.

Either way, something to think about for getting a local integration test setup.

Suggestions/Questions:

1. output.nc --> define the output variables
2. Describe what the different directories/files contain
3. Passed by reference documentation - What was done when a variable pbr name change was detected? Which variable names were chosen for use in Modular Noah-MP? -- basically the documentation seems incomplete, stopping after the detection and not continuing with what was done after.

I have also included a set of more detailed instructions for the current 'full' noah-mp-modular.
Step_by_step_instructions-Modular_NOAH-MP_full_directory.docx

... collecting all basic earth system or other needed constants and setting them as [type], parameter ...

Current behavior

When encountering an error, NoahOM prints an error message to console immediately prior to stopping the model with a call to stop. However, because the call to stop prevents the BMI update or update_until functions from finishing, NextGen is unable to report time and location (i.e., catchment id) information to the model-user along with the error message.

Expected behavior

When encountering an error, NoahOM should return to the BMI update or update_until functions and indicate that an error has occurred along with any recorded error messaging.

Suggested changes

• Remove calls to stop in the NoahOM code base
• When encountering an error, save the error message and record that an error has occurred using a flag/logical variable
• Add code to BMI update function to check for an error flag value and returns BMI_FAILURE and writes the message
• Use return rather than stop

Notes

• This NoahOM issue originates from NextGen issue 723

The output functionality is a carryover from previous work. We need to check the units, variable names, and add output vars.

@lcunha0118 noticed that you can run Noah-OWP-Modular with a namelist file that's missing entries. To test this, I ran the model through initialize() and printed out results of the terrain_slope parameter.

• Scenario 1 (correct namelist): Model initializes, namelist and parameter have same correct value (10°)
  
• Scenario 2 (namelist missing terrain_slope): Model initializes, namelist and parameter have same zero value
  
• Scenario 3 (namelist missing terrain_slope with fake entry added in its place): Model crashes, error is reported
  

We should implement a routine that stops the model from running when a namelist key is missing, similar to the error that's reported when an incorrect key is provided.

Current behavior

To build Noah-OWP-Modular currently, you need to:

1. run ./configure
2. hope your NetCDF and compiler info matches one of the preconfigured options or edit user_build_options
3. run make

Suggested changes

Create a cmake build system that doesn't rely on hard-coded compiler and NetCDF paths.

Noah-OWP-Modular currently relies on air temperature alone to determine precipitation phase. These methods will induce inaccuracies in modeled SWE. See Jennings et al. (2018) and Jennings and Molotch (2019) for more info.

We should considering adding one or two additional methods:

1. A wet bulb temperature threshold
2. A binary logistic regression where phase is predicted as f(air temperature, relative humidity)

Additionally, the air temperature thresholds are hard-coded. We should consider moving them to the namelist.

To incorporate these changes, we will need to modify:

• src/AtmProcessing.f90
• src/NamelistRead.f90 (if adding threshold as a parameter)
• src/ParametersType.f90 (if adding threshold as a parameter)
• src/OptionsType.f90 (comment documentation)
• run/namelist.input

Current behavior

You can run Noah-OM with nonsensical, inconsistent soil and land cover values. As we saw in NOAA-OWP/DMOD#472 (comment), this can lead to model errors.

Expected behavior

Noah-OM should not initialize if inconsistent values of land cover and soil are provided.

Suggested changes

I've flagged an issue on the hydrofabric repo, but we should also implement model-level sanity checks. These will stop initialization and throw an error message if land cover and soil are inconsistent.

Specifically, Noah-OM should check the values of isltyp and vegtyp for consistency. I provide example category lumpings in the linked hydrofabric issue above.

Hi @SnowHydrology and @GreyEvenson-NOAA,

Looking through the grid implementation (which looks great!), I noticed most of use statements do not have only,so just pointing to the module. By explicitly specifying public entities (subroutine, function or variables etc.) with only, it is easier to tell where the entities are coming from, and easier to trace the code, and also allow to avoid name conflict (can rename referred entities. As the codes get complex, this helps maintainability, code documentation without comments.

something to think about...

https://stackoverflow.com/questions/51686745/why-should-i-use-use-only-in-fortran

With the following cmake configuration:

$ cmake ..
-- The Fortran compiler identification is GNU 12.2.0
-- Checking whether Fortran compiler has -isysroot
-- Checking whether Fortran compiler has -isysroot - yes
-- Checking whether Fortran compiler supports OSX deployment target flag
-- Checking whether Fortran compiler supports OSX deployment target flag - yes
-- Detecting Fortran compiler ABI info
-- Detecting Fortran compiler ABI info - done
-- Check for working Fortran compiler: gfortran - skipped
-- The C compiler identification is AppleClang 14.0.0.14000029
-- The CXX compiler identification is AppleClang 14.0.0.14000029
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: cmake_build

I get the following build error when running make in the build directory:

noah-owp-modular/src/DomainType.f90:110:48:

  110 |     this%terrain_slope  = namelist%terrain_slope
      |                                                1
Error: 'terrain_slope' at (1) is not a member of the 'namelist_type' structure
noah-owp-modular/src/DomainType.f90:111:42:

  111 |     this%azimuth        = namelist%azimuth
      |                                          1
Error: 'azimuth' at (1) is not a member of the 'namelist_type' structure

Current behavior

Noah-MP-Modular is split into three separate modules: full, surface, and surface_bmi. If code is modified in one module, then the corresponding code has to be modified in the others.

Desired behavior

Implement the hydrostatic subsurface as an option in full to reduce the amount of edits that need to be made when code is modified.

Work on the current NWM identified Fortran goto statements as a potential issue. Modern Fortran has moved away from goto statements for myriad reasons, some of which are discussed here.

There are three modules from which the gotos should be removed (https://github.com/NOAA-OWP/noah-owp-modular/search?q=goto):

• SurfaceRunoffInfiltration.f90
• SnowSoilTempModule.f90
• AlbedoModule.f90

Alternative solutions include do loops, if statements, cycle, exit, and case.

This issue also needs fixing in Sac-SMA and Snow-17.

Needed to test code more rigorously and set up for BMI links

Move and update old unit test script once get_value_ptr functions are implemented.

Here: https://github.com/NOAA-OWP/noah-owp-modular/tree/d3ca9775bc861e4ab832cc0f590aa775dbbffa5b/modules/surface_bmi/test

When running in the Nextgen model engine, we should have flags that allow forcing data and output functionalities to be ignored so that IO is handled exclusively by Nextgen. There is currently an NGEN_ACTIVE flag set for the BMI specification, so we should add granular flags that refer to forcing and output.

The TG variable in Noah-OWP-Modular is nominally ground temperature, but it looks like it becomes snow surface temperature when there is snow on the ground. This means we no longer have information on ground surface temperature that we can pass to a subsurface module such as Soil Freeze Thaw.

To do: Make a new scalar variable that is equal to TG when there is no snow on the ground and equal to the lowest snow element temperature STC[0] when there is snow on the ground.

Need to change:

• src/EnergyType.f90 (new var that takes the TG or STC[0] value)
• src/EnergyModule.f90 (assigns value to new var in if-else statement)
• bmi/bmi_noahowp.f90 (specifies var info for passing via BMI)

Currently, the 1D Noah-OWP-Modular driver reads in precipitation rate, which it assigns to PRCP in the AsciiReadModule and then to PRCPNONC in AtmProcessing. For Nextgen compatibility, there is a compiler directive in AtmProcessing that toggles between PRCP and PRCPNONC depending on the NGEN_FORCING_ACTIVE flag.

We should allow the driver to accept constituent types of precipitation for when/if it's forced by a weather model that provides multiple forms of precipitation.

We should also evaluate whether computing the fraction of a grid cell receiving precipitation (water%fp) is appropriate for smaller scale modeling.

The framework may offer calibration functionality that uses BMI to set param values, but it would not be hard to write a driver for a standalone-mode calibration using some common optimization algorithms.

This driver could initialize the model, perform some initial spinup runs as needed, the iterate through a param update, run, and evaluate cycle, calling an optimization routine, before finalizing. It would use the power of BMI set_value for parameters rather than the traditional update of param input files and restart of the model.

I am trying to generate noah owp modular configuration files from linked data in the hydrofabric, specifically the cfe_noahowp_attributes table. The generated config files will be used to run noah owp modular using NGEN. For reference, I am using hydrofabric v1.2. Having used the cfe_noahowp_attributes schema outlined here, looked through the fortran option source code, and referred to the example namelist in /run as a guidance, i've determined most of the "mappings", but I am not sure how to generate the below configuration options. Sane defaults to any of these options or derivations from other attributes would also suffice! Thanks for the help in advance!

Configuration options in question:

• forcing.rain_snow_thresh - rain-snow temperature threshold (degrees Celcius)
• structure.soilcolor - soil color code
• initial_values.dzsnso - level thickness [m]
• initial_values.sice - initial soil ice profile [m3/m3]
• initial_values.sh2o - initial soil liquid profile [m3/m3]
• initial_values.zwt - initial water table depth below surface

To hopefully make this a little easier, here is the Hydrofabric v1.2 cfe_noahowp_attributes schema i've been using as reference:

Also worth noting, IVGTYP's are the USGS 27 code variant and ISLTYP comes from STAS.

There is currently no internal error handling for out-of-range options in Noah-OWP-Modular. This means we can enter a non-sensical value for an option flag and the model would still try to run without alerting us of an issue.

We should implement code, likely in NamelistRead.f90 that checks each option to make sure the provided value is within a valid range. If the option is not in range, then the model should stop running and return a clear message. In pseudo code:

if option_val < 0 or option_val > option_max then
    print "option_val for option_name out of range, exiting..."
    stop
end if

current date/time handling is based on strings, which is brittle and prevents using most fortran time utilties and conventions

Consider adding additional diagnostic variables to surface_bmi output_vars so they can be written to Nextgen output.

There are significant concerns over the similarity between the current name and 'Noah-MP', which is well-known in its community and has a code base / repo in another organization, NCAR. Right now, the parent code of Noah-MP exists as 'NoahLSM' at EMC -- https://github.com/NOAA-EMC/NoahLSM -- which does not apparently raise concerns about similarity.

The code contents and functionality will likely diverge steadily from the original Noah-MP components, and will not be backward compatible or ultimately closely related to Noah-MP; though it will always be a model that is from the Noah model tree (which began as the OSU model). Similarly the Stanford Watershed Model from the 1960s later spawned other models such as EPA's HSPF and the NWS Sacramento model, which took on other names. I suggest a name that includes 'Noah' to recognize the heritage, but not 'Noah-MP', which may soon be trademarked.

This re-branding will avoid confusion in talking about Noah-MP versus, say, 'Noah-modular' even during the development stage.

Current behavior

Calling bmi_noahowp%get_grid_spacing returns (/model%domaingrid%dy, model%domaingrid%dx/) (i.e., via bmi_grid%spacing). This behavior is consistent with BMI 2.0 specifications. However, as the units of dx and dy are not specified in the BMI 2.0 documentation, there is ambiguity regarding the appropriateness of the current implementation.

When executed in standalone (outside NextGen), dx and dy are read-into the model via the required domain attributes NetCDF file (which is expected to have global attributes dx and dy). The example attributes file (i.e., data/NorthForkAttributes.nc) contains a subset of the NWM 3.0 grid for the North Fork (WY) CAMELS test basin; hence, dx = dy = 1 km.

Suggested changes

Verify current implementation of bmi_noahowp%get_grid_spacing or amend implementation of dx and dy as needed.

Noah-OWP-Modular assumes its model domain is flat. This means it cannot resolve the effects of slope and aspect on hydrologic processes modulated by incoming solar radiation (e.g. snowmelt, evapotranspiration). This should be rectified.

We will need to modify the following files to incorporate the effects of slope and aspect on solar radiation. The main change will be to produce a new cosine of the solar zenith angle (COSZ) value.

-src/UtilitiesModule.f90 (modify equations for COSZ)
-src/NamelistRead.f90 (read in slope/aspect)
-src/DomainType.f90 (add slope/aspect)
-run/namelist.input (add slope/aspect)