search

NOAA-OWP / cfe

CFE is a conceptual rainfall-runoff model with an implementation of the Basic Model Interface.
Other
20 stars 21 forks source link

variable_names_changes #116

Closed ajkhattak closed 3 months ago

ajkhattak commented 5 months ago

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

Removals

Changes

Testing

  1. Results with and without changes are identical

Checklist

PhilMiller commented 5 months ago

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 commented 5 months ago

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 https://github.com/NOAA-OWP/cfe/pull/116#issuecomment-2088915329, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKAROGFDOWLGUCD3WT3GDZAE2NHAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYHEYTKMZSHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

stcui007 commented 5 months ago

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 https://github.com/NOAA-OWP/cfe/pull/116#issuecomment-2088915329, 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 https://github.com/NOAA-OWP/cfe/pull/116#issuecomment-2089175884, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACA4SROGGQ2K2TQQ25FFW7DZAFN4JAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZGE3TKOBYGQ . You are receiving this because you were mentioned.Message ID: @.***>

ajkhattak commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 'surfacerunoff'? … <#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) https://github.com/NOAA-OWP/cfe/pull/116#issuecomment-2088915329>, 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 https://github.com/NOAA-OWP/cfe/pull/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 https://github.com/NOAA-OWP/cfe/pull/116#issuecomment-2090643309, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIKARKK5WIEOAMOC4CCQP3ZAJFBRAVCNFSM6AAAAABHCIARRGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQGY2DGMZQHE . You are receiving this because you were mentioned.Message ID: @.***>

aaraney commented 5 months ago

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 commented 5 months ago

@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 commented 4 months ago

@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 commented 3 months ago

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