variable_names_changes

[ajkhattak]

As a result of the updated surface runoff schemes (PR #112), certain surface runoff terminologies, such as direct_runoff, required revision. This PR adjusted several variable names to more suitable names. No changes to the functionality of the model.

Additions

• Updated some variables names

Removals

• Removed a few comments that were not needed

Changes

• changed direct_runoff_parameters_structure to infiltraton_excess_parameters_structure
• changed surface_partitioning_scheme to surface_water_partitioning_scheme
• changed flux_output_direct_runoff to flux_infiltration_excess
• changed surface_runoff to direct_runoff
• changed direct_output_runoff_m to infiltration_excess_m
• changed BMI name: DIRECT_RUNOFF to INFILTRATION_EXCESS
• changed BMI name: SURFACE_RUNOFF to DIRECT_RUNOFF

Testing

1. Results with and without changes are identical

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
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

[PhilMiller]

Changes in BMI variable names are presumably going to require corresponding changes in Realization Config files, to map them appropriately between coupled models. This presumably affects both the documentation and examples @stcui007 is developing, and the config generation in DMOD that I think @aaraney maintains.

[andywood]

I'm curious about the change of surface_runoff (a very common term in hydrology) to direct_runoff (one I haven't heard before). Can you comment on the issue with 'surface_runoff'?

…

On Wed, May 1, 2024 at 12:49 PM Phil Miller - NOAA ***@***.***> wrote: Changes in BMI variable names are presumably going to require corresponding changes in Realization Config files, to map them appropriately between coupled models. This presumably affects both the documentation and examples @stcui007 <https://github.com/stcui007> is developing, and the config generation in DMOD that I think @aaraney <https://github.com/aaraney> maintains. — Reply to this email directly, view it on GitHub <#116 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[stcui007]

On the Ngen side, this change would make all the current initial configurations unusable. We would have to redo all our tests again, at a very large cost for CONUS simulations.

…

On Wed, May 1, 2024 at 4:36 PM Andy Wood ***@***.***> wrote: I'm curious about the change of surface_runoff (a very common term in hydrology) to direct_runoff (one I haven't heard before). Can you comment on the issue with 'surface_runoff'? On Wed, May 1, 2024 at 12:49 PM Phil Miller - NOAA ***@***.***> wrote: > Changes in BMI variable names are presumably going to require > corresponding changes in Realization Config files, to map them > appropriately between coupled models. This presumably affects both the > documentation and examples @stcui007 <https://github.com/stcui007> is > developing, and the config generation in DMOD that I think @aaraney > <https://github.com/aaraney> maintains. > > — > Reply to this email directly, view it on GitHub > <#116 (comment)>, or > unsubscribe > < https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE> > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> > — Reply to this email directly, view it on GitHub <#116 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SROGGQ2K2TQQ25FFW7DZAFN4JAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZGE3TKOBYGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ajkhattak]

Changes in BMI variable names are presumably going to require corresponding changes in Realization Config files, to map them appropriately between coupled models. This presumably affects both the documentation and examples @stcui007 is developing, and the config generation in DMOD that I think @aaraney maintains.

@PhilMiller that's a good point, all models taking DIRECT_RUNOFF from CFE as an input should continue to do so, and I don't think INFILTRATION_EXCESS is ever used by other models, so hopefully it will not impact realization files and also coupling between models. Please let me know if I am missing something here

From the outputs perspective for visualization, prior to April 18th, those who were outputting 'GIUH_RUNOFF' will need to output DIRECT_RUNOFF and DIRECT_RUNOFF needs to be INFILTRATION_EXCESS. The name SURFACE_RUNOFF was introduced in the last PR, which is reverted here to DIRECT_RUNOFF. I will add this to the PR description for clarification.

[aaraney]

@PhilMiller that's a good point, all models taking DIRECT_RUNOFF from CFE as an input should continue to do so, and I don't think INFILTRATION_EXCESS is ever used by other models, so hopefully it will not impact realization files and also coupling between models. Please let me know if I am missing something here

I think the main concern, @ajkhattak is changing surface_partitioning_scheme to surface_water_partitioning_scheme in cfe's configuration file. This is not a backwards compatible change, so any existing cfe "init config" files would no longer work (@stcui007's point). Is there a need to change the init config variable name?

[ajkhattak]

@PhilMiller that's a good point, all models taking DIRECT_RUNOFF from CFE as an input should continue to do so, and I don't think INFILTRATION_EXCESS is ever used by other models, so hopefully it will not impact realization files and also coupling between models. Please let me know if I am missing something here

I think the main concern, @ajkhattak is changing surface_partitioning_scheme to surface_water_partitioning_scheme in cfe's configuration file. This is not a backwards compatible change, so any existing cfe "init config" files would no longer work (@stcui007's point). Is there a need to change the init config variable name?

@aaraney surface_water_partitioning_scheme is a more descriptive and appropriate name because these schemes does partition surface water (precipitation or snowmelt water) into infiltration and runoff. That said, we can keep the old name surface_partitioning_scheme for a while, which will be deprecated in the future. does it make sense?

[aaraney]

@aaraney surface_water_partitioning_scheme is a more descriptive and appropriate name because these schemes does partition surface water (precipitation or snowmelt water) into infiltration and runoff. That said, we can keep the old name surface_partitioning_name for a while, which will be deprecated in the future. does it make sense?

I didn't speculate, but I figured that was the case! I think a deprecation path is best if we need to change the name, although I wish we could just avoid it all together. I suggest that we make it possible to use surface_partitioning_name but output a warning if it is present. Ideally we would provide a way to upgrade existing configs to the "new" naming convention as well. Perhaps we could create a gh issue that showcases how to upgrade using something like sed or awk and we output a link to the gh issue in the warning. This was there is room for conversation and others to link back to this breaking change. @PhilMiller, do you have other thoughts?

[ajkhattak]

@aaraney @stcui007, according to @fred-ogden the revised CFE (including all these changes) is cfe3.0 and is conceptually more "equivalent" to the existing NWM. The cfe3.0 requires new inputs from the config file N_nash_surface, K_nash_surface, and nash_storage_surface, thus anyone running cfe3.0 will need to regenerate their config files irrespective of the other changes (surface_water_partitioning_scheme or surface_partitioning_scheme) 😊

[ajkhattak]

@aaraney surface_water_partitioning_scheme is a more descriptive and appropriate name because these schemes does partition surface water (precipitation or snowmelt water) into infiltration and runoff. That said, we can keep the old name surface_partitioning_name for a while, which will be deprecated in the future. does it make sense?

I didn't speculate, but I figured that was the case! I think a deprecation path is best if we need to change the name, although I wish we could just avoid it all together. I suggest that we make it possible to use surface_partitioning_name but output a warning if it is present. Ideally we would provide a way to upgrade existing configs to the "new" naming convention as well. Perhaps we could create a gh issue that showcases how to upgrade using something like sed or awk and we output a link to the gh issue in the warning. This was there is room for conversation and others to link back to this breaking change. @PhilMiller, do you have other thoughts?

We can definitely find a way to keep your existing files (configs and realization files) useable. I agree with you probably deprecation is the best way forward here

[ajkhattak]

I'm curious about the change of surface_runoff (a very common term in hydrology) to direct_runoff (one I haven't heard before). Can you comment on the issue with 'surface_runoff'?
…
On Wed, May 1, 2024 at 12:49 PM Phil Miller - NOAA @.> wrote: Changes in BMI variable names are presumably going to require corresponding changes in Realization Config files, to map them appropriately between coupled models. This presumably affects both the documentation and examples @stcui007 https://github.com/stcui007 is developing, and the config generation in DMOD that I think @aaraney https://github.com/aaraney maintains. — Reply to this email directly, view it on GitHub <#116 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE . You are receiving this because you are subscribed to this thread.Message ID: @.>

@andywood historically CFE never used the term SURFACE_RUNOFF which I introduced in the PR #112, and trying to revert it here to its old name DIRECT_RUNOFF, and I agree it is very commonly used term. However, according to the revised CFE schematic that Fred put together, the term DIRECT_RUNOFF refers to the portion of surface water that goes directly into streams in a timestep. When we route infiltration excess water through GIUH or Nash Cascade, generally speaking, not all of it makes it to the channel within a single timestep, so whatever drains in that timestep is the direct_runoff and the rest is the surface water (which will drain the next timestep; all or portion of it).

Based on my understanding, the term surface runoff generally refers to the overland flow, but note I am not a hydrologist 😊. I can setup a short call to discuss it offline a bit more

[PhilMiller]

@aaraney @stcui007, according to @fred-ogden the revised CFE (including all these changes) is cfe3.0 and is conceptually more "equivalent" to the existing NWM. The cfe3.0 requires new inputs from the config file N_nash_surface, K_nash_surface, and nash_storage_surface, thus anyone running cfe3.0 will need to regenerate their config files irrespective of the other changes (surface_water_partitioning_scheme or surface_partitioning_scheme) 😊

If those new configuration variables are required in the input file, then I think I'd be more inclined not to go through a deprecation process for the surface(_water)_partitioning_scheme variable. Rather, its presence can be taken as a signal of an 'old' config file, and we can error saying "CFE 3.0 requires config file updates. See #issue for details".

[andywood]

@ajkhattak, thanks for the explanation! On Thu, May 2, 2024 at 8:32 AM AJKhattak-NOAA ***@***.***> wrote:

…

I'm curious about the change of surface_runoff (a very common term in hydrology) to direct_runoff (one I haven't heard before). Can you comment on the issue with 'surface_runoff'? … <#m_-470798467006399107_> On Wed, May 1, 2024 at 12:49 PM Phil Miller - NOAA *@*.*> wrote: Changes in BMI variable names are presumably going to require corresponding changes in Realization Config files, to map them appropriately between coupled models. This presumably affects both the documentation and examples @stcui007 <https://github.com/stcui007> https://github.com/stcui007 <https://github.com/stcui007> is developing, and the config generation in DMOD that I think @aaraney <https://github.com/aaraney> https://github.com/aaraney <https://github.com/aaraney> maintains. — Reply to this email directly, view it on GitHub <#116 (comment) <#116 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE <https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE> . You are receiving this because you are subscribed to this thread.Message ID: @.*> @andywood <https://github.com/andywood> historically CFE never used the term SURFACE_RUNOFF which I introduced in the PR #112 <#112>, and trying to revert it here to its old name DIRECT_RUNOFF, and I agree it is very commonly used term. However, according to the revised CFE schematic that Fred put together, the term DIRECT_RUNOFF refers to the portion of the surface water that goes directly into the streams in a timestep. When we route infiltration excess water through GIUH or Nash Cascade, generally speaking, not all of it makes it to the channel within a single timestep, so whatever drain in that timestep is the direct_runoff and the rest is the surface water (which will drain the next timestep; all or portion of it). Based on my understanding, the term surface runoff generally refers to the overland flow, but note I am not a hydrologist 😊. I can setup a short call to discuss it offline a bit more — Reply to this email directly, view it on GitHub <#116 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIKARKK5WIEOAMOC4CCQP3ZAJFBRAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQGY2DGMZQHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[aaraney]

Just to make it easy to distinguish CFE 2.x from CFE 3.0, lets make sure to tag the commit at HEAD just prior to merging this. @ajkhattak and I chatted about doing this in a side channel, just leaving a todo here.

[ajkhattak]

@aaraney to be more specific, we will tag this 0be829df6777c42b62993e396dfcb485adf28aae commit as cfe2.0, as even the previous commit has changes related to cfe3.0

[madMatchstick]

@ajkhattak - I was planning on making list of places where this CFE PR would effect other formulations as well as ngen (e.g. realization.json with the BMI name changes.) Would this be helpful? -thanks!

[aaraney]

Spelling fix!

Suggested change

	| surface_water_partitioning_scheme | *char* | `Xinanjiang` or `Schaake` | | parameter_adjustable | infiltraton exces | |
	| surface_water_partitioning_scheme | *char* | `Xinanjiang` or `Schaake` | | parameter_adjustable | infiltration excess | |

[aaraney]

For history sake, it looks like these changes were introduced in #120.