Remove deprecated Forcing class dependency from legacy formulations #432

stcui007 · 2022-08-11T19:30:30Z

Removing the old legacy forcing

Additions

Removals

Changes

include/realizations/catchment/Catchment_Formulation.hpp
include/realizations/catchment/Formulation_Constructors.hpp
include/realizations/catchment/Simple_Lumped_Model_Realization.hpp
include/realizations/catchment/Tshirt_C_Realization.hpp
include/realizations/catchment/Tshirt_Realization.hpp
src/realizations/catchment/Simple_Lumped_Model_Realization.cpp
src/realizations/catchment/Tshirt_C_Realization.cpp
src/realizations/catchment/Tshirt_Realization.cpp
test/realizations/catchments/Bmi_C_Cfe_IT.cpp
test/realizations/catchments/Bmi_C_Formulation_Test.cpp
test/realizations/catchments/Bmi_Cpp_Formulation_Test.cpp
test/realizations/catchments/Bmi_Fortran_Formulation_Test.cpp
test/realizations/catchments/Bmi_Py_Formulation_Test.cpp
test/realizations/catchments/LSTM_Realization_Test.cpp

Testing

Screenshots

Notes

Todos

Checklist

Testing checklist (automated report can be put here)

Target Environment support

Linux

mattw-nws

This is an intitial pass... I haven't checked into the logic for doing the epoch time differentials, for instance. But a few changes I already see.

Also, generally, go ahead and delete commented-out references to Forcing.h and _link_legacy_forcing(), we can see what it was previously via Git.

mattw-nws · 2022-08-11T19:46:23Z

include/realizations/catchment/Catchment_Formulation.hpp

@@ -7,7 +7,8 @@
 #include "Et_Accountable.hpp"
 #include <HY_CatchmentArea.hpp>

-#include <Forcing.h> // Remove when _link_legacy_forcing() is removed!
+//#include <Forcing.h> // Remove when _link_legacy_forcing() is removed!
+#include "CsvPerFeatureForcingProvider.hpp"


This should not be here. Not sure if anything should go in it's place, but if anything maybe DataProvider.hpp?

Some of the time functions, get_data_start_time() and several others, for example are only provided in the following header files, so we need to provide CsvPerFeatureForcingProvider some where. I just tried to use GenericDataProvider.hpp and DataProvider.hpp in its place, both gave build errors.

include/forcing/CsvPerFeatureForcingProvider.hpp: long get_data_start_time() override { include/forcing/DataProvider.hpp: virtual long get_data_start_time() = 0; include/forcing/WrappedForcingProvider.hpp: return wrapped_provider->get_data_start_time(); include/forcing/WrappedForcingProvider.hpp: long get_data_start_time() override { include/forcing/WrappedForcingProvider.hpp: return wrapped_provider->get_data_start_time(); include/forcing/Forcing.h: long get_data_start_time() override include/forcing/NetCDFPerFeatureDataProvider.hpp: long get_data_start_time()

Confused on this, since as you show above, get_data_start_time() is defined in both, and it's not even templated... will try this myself and have a look.

It seems to me the get_data_start_time() is only accessible through Forcing, CsvPerFeatureForcingProvider, WrappedForcingProvider, or NetCDFPerFeatureDataProvider.

You maybe right. Probably need some code reorganization.

The issue here doesn't seem to have anything to do with get_data_start_time(). See the comment in Formulation_Constructors.hpp.

mattw-nws · 2022-08-11T19:48:49Z

include/realizations/catchment/Formulation_Constructors.hpp

@@ -72,7 +72,8 @@ namespace realization {
        std::shared_ptr<data_access::GenericDataProvider> fp;
        if (formulation_type == "tshirt"  || formulation_type == "tshirt_c"  || formulation_type == "lstm" // These formulations are still using the legacy interface!
            ){
-            fp = std::make_shared<Forcing>(forcing_config);
+            //fp = std::make_shared<Forcing>(forcing_config);


The if block above here is an exception that prevented the use of CsvPerFeatureForcingProvider below. Rather than this change, this conditional block should be reduced or removed.

I'll try to reduce it. fp will still need to have a value though, as in the netCDF forcing provider a couple lines below.

^ Exactly. What I mean is, remove tshirt_c and tshirt from the conditional above... they should fall through to the other blocks, depending on what the forcing_config says to use. They shouldn't be locked to CsvPerFeature, which is what the code does now.

Removing tshirt and tshirt_c appears to cause the Formulation_Manager_Test to fail. There are a bunch of tshirt and tshirt_c examples there using the old form of forcing explicitly. Do we still need the Formulation_Manager_Test.cpp?

Fix the provider string in Formulation_Manager_Test.cpp:

"\"forcing\": { " "\"provider\": \"legacy\"," "\"path\": \"./data/forcing/cat-67_2015-12-01 00_00_00_2015-12-30 23_00_00.csv\" "

mattw-nws · 2022-08-11T20:01:03Z

src/realizations/catchment/Tshirt_C_Realization.cpp

@@ -37,13 +39,12 @@ Tshirt_C_Realization::Tshirt_C_Realization(forcing_params forcing_config,
                                           std::vector<double> giuh_ordinates,
                                           tshirt::tshirt_params params,
                                           const vector<double> &nash_storage)
-        : Catchment_Formulation(catchment_id, std::move(std::make_unique<Forcing>(forcing_config)), output_stream), catchment_id(std::move(catchment_id)),
+        //: Catchment_Formulation(catchment_id, std::move(std::make_unique<Forcing>(forcing_config)), output_stream), catchment_id(std::move(catchment_id)),
+        : Catchment_Formulation(catchment_id, std::move(std::make_unique<CsvPerFeatureForcingProvider>(forcing_config)), output_stream), catchment_id(std::move(catchment_id)),


I'm betting this should not be specific to the CSV reader... where is this constructor used? Maybe this is only in test code and that's okay, but not sure.

It looks like the class is use by a few testing codes. Not sure which form of the constructors.

mattw-nws · 2022-08-11T20:01:52Z

src/realizations/catchment/Tshirt_C_Realization.cpp

-) : Catchment_Formulation(std::move(id), std::move(std::make_unique<Forcing>(forcing_config)), output_stream) {
-    _link_legacy_forcing();
+//) : Catchment_Formulation(std::move(id), std::move(std::make_unique<Forcing>(forcing_config)), output_stream) {
+) : Catchment_Formulation(std::move(id), std::move(std::make_unique<CsvPerFeatureForcingProvider>(forcing_config)), output_stream) {


Same comment as above... where is this constructor used, and does this need to be non-specific to CsvPerFeature?

I would tend to agree...

mattw-nws · 2022-08-11T20:04:35Z

src/realizations/catchment/Tshirt_C_Realization.cpp

+    if (t_index < 0) {
+        throw std::invalid_argument("Getting response of negative time step in Tshirt C Realization is not allowed.");
+    }
+    time_t start_time = this->forcing->get_data_start_time();


While time_t is probably reasonable here, I know in some other places we have used long because we may deal with dates < 1970. Is this based on code in another module?

time_t is used all over the places in our codes. In CsvPerFeatureForcingProvider.hpp, time_t is used for epoch time. This is used here for consistency. If you think that might not be enough, we can change that.

mattw-nws · 2022-08-11T20:08:57Z

src/realizations/catchment/Tshirt_Realization.cpp

@@ -3,6 +3,8 @@
 #include "TshirtErrorCodes.h"
 #include "Catchment_Formulation.hpp"
 using namespace realization;
+using data_access::MEAN;


This one isn't actually used, right? And for SUM, I would skip the using and just use the namespaced name below in the one call it appears in.

Mean is not used here. Will make the suggested change for SUM.

mattw-nws · 2022-08-11T20:11:13Z

src/realizations/catchment/Tshirt_C_Realization.cpp

@@ -9,6 +9,8 @@
 #include <boost/algorithm/string/join.hpp>

 using namespace realization;
+using data_access::MEAN;


See comments in Tshirt_Realization.cpp on these

mattw-nws · 2022-08-11T20:14:25Z

test/realizations/catchments/LSTM_Realization_Test.cpp

@@ -2,7 +2,7 @@

 #include "gtest/gtest.h"
 #include "realizations/catchment/LSTM_Realization.hpp"
-#include "forcing/Forcing.h"
+//#include "forcing/Forcing.h"


Note that these tests are almost certainly not actually being run because of the guard at line 1...I am betting that if we can run these tests they won't succeed with only this change.

At the moment, Ngen framework does not seem to set up to handle LSTM module. So there is actually no test code ever built because of the guard lines.

mattw-nws · 2022-08-11T20:21:57Z

include/forcing/AorcForcing.hpp

@@ -14,6 +14,18 @@
 #define CSDMS_STD_NAME_WIND_V_Y "land_surface_wind__y_component_of_velocity"
 #define NGEN_STD_NAME_SPECIFIC_HUMIDITY "atmosphere_air_water~vapor__relative_saturation" // This is not present in standard names, use this for now... may change!

+// Recognized Forcing Value Names (in particular for use when configuring BMI input variables)
+// TODO: perhaps create way to configure a mapping of these to something different
+#define AORC_FIELD_NAME_PRECIP_RATE "precip_rate"


Interestingly... these defines appear to only be used in test code... though the magic strings appear elsewhere (in this file even)... no action necessary here now, but this is probably something that should be refactored, some way, at some point.

Yeah, need to keep in mind.

mattw-nws · 2022-08-12T20:38:31Z

include/realizations/catchment/Formulation_Constructors.hpp

@@ -18,7 +18,7 @@
 #include "Bmi_Multi_Formulation.hpp"
 #include "Bmi_Py_Formulation.hpp"
 #include <GenericDataProvider.hpp>
-#include "CsvPerFeatureForcingProvider.hpp"
+//#include "CsvPerFeatureForcingProvider.hpp"


This change is actually what is causing your problem in Catchment_Formulation.hpp. This is ~~the~~ one place that CsvPerFeatureForcingProvider.hpp is needed, because the instances of the providers are instantiated in this class.

You will need #include "CsvPerFeatureForcingProvider.hpp" in files where CsvPerFeatureForcingProvider is explicitly constructed... for the moment that means the two tshirt files also (though ideally those dependencies can be removed, per the discussion there).

mattw-nws

The only problem left has to do with LSTM_Realization... either it needs to be fixed, dropped, or this will have to keep Forcing.h for that one realization... hold until decided.

mattw-nws · 2022-08-18T20:40:06Z

include/realizations/catchment/Formulation_Constructors.hpp

-        if (formulation_type == "tshirt"  || formulation_type == "tshirt_c"  || formulation_type == "lstm" // These formulations are still using the legacy interface!
-            ){
-            //fp = std::make_shared<Forcing>(forcing_config);
+        if (formulation_type == "lstm") {
            fp = std::make_shared<CsvPerFeatureForcingProvider>(forcing_config);


This unfortunately should not stay this way. This will break LSTM_Realization it seems, so this will either need to go back to Forcing or both LSTM_Realization and this whole block should be dropped. Posted a question in Slack on which to do.

mattw-nws · 2022-08-18T20:55:04Z

src/realizations/catchment/Tshirt_Realization.cpp

+    time_t stop_time = this->forcing->get_data_stop_time();
+    time_t t_current = start_time + t_index * t_delta_s;
+    //Ensure model run does not exceed the end time of forcing
+    if (t_current > stop_time) {


Interestingly, the BMI formulations allow this... but code suggests it was never a feature for other realizations... so this is probably fine.

mattw-nws · 2022-08-18T20:58:24Z

src/realizations/catchment/Tshirt_Realization.cpp

-    double precip = this->legacy_forcing->get_next_hourly_precipitation_meters_per_second();
+    time_t t_delta = this->forcing->record_duration();
+    if (t_delta != t_delta_s) {    //Checking the time step used is consistent with that provided in forcing data
+        throw std::invalid_argument("Getting response using insonsistent time step with provided forcing data");


This is definitely legal for BMI realizations... however, apparently it wasn't under the old API (they had to be in sync), so this is fine for any current use cases.

stcui007 · 2022-08-24T20:16:14Z

I have just pushed my codes to my fork.with the branch bane cui_esmpy

…

On Wed, Aug 24, 2022 at 1:44 PM Matt Williamson ***@***.***> wrote: @stcui007 <https://github.com/stcui007> Can you repackage this as a PR to https://github.com/NOAA-OWP/ngen-forcing please--I'd like to move these efforts over there while there in a higher state of flux/earlier state of development. Just put it in a directory with an appropriate name for now... things in there will probably get moved around as things mature. — Reply to this email directly, view it on GitHub <#432 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRPUNVQZ5YL3KGQ6BRLV2ZUPJANCNFSM56JJ3L4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…east.

hellkite500 · 2022-09-12T14:04:53Z

src/realizations/catchment/LSTM_Realization.cpp

+    }
+    //Negative t_index is not allowed
+    if (t_index < 0) {
+        throw std::invalid_argument("Getting response of negative time step in Tshirt C Realization is not allowed.");


This is LSTM_Realization, not Tshirt C Realization

hellkite500 · 2022-09-12T14:09:21Z

src/realizations/catchment/LSTM_Realization.cpp

+    }
+    time_t start_time = this->forcing->get_data_start_time();
+    time_t stop_time = this->forcing->get_data_stop_time();
+    time_t t_current = start_time + t_index * t_delta_s;


is it guaranteed that data start time will align with simulation start time? Also, there is no guarantee that t_delta_s is the SAME delta at each call to get response.

And I'm not sure I'm tracking time here correctly...

t_index should be the current_time relative to the simulation driver (or whatever is calling get_response. t_delta_s is the amount of seconds the call wants to advance relative to t_index, or current_time. Is this just trying to convert that into time into the forcing provider's time reference?

as this code is for the generic provider the forcing time step could be anything, it does not necessarily match the model time step or is covenant multiple of the model time step. At least for all existing providers it is not a dynamic value.

is it guaranteed that data start time will align with simulation start time?

DP interface does support retrieving arbitrary time slices...however, this legacy code was already fixed to the forcing timestep by virtue of the fact that the old Forcing object kept the timestep and time state--that has shifted out of the DataProvider interface, but this didn't add any new restrictions.

Also, there is no guarantee that t_delta_s is the SAME delta at each call to get response.

Perhaps not via the interface, but the rest of the framework is currently locked at 3600. That's going to require way more complex changes than you'll find here to change.

hellkite500 · 2022-09-12T14:10:57Z

src/realizations/catchment/Tshirt_C_Realization.cpp

-) : Catchment_Formulation(std::move(id), std::move(std::make_unique<Forcing>(forcing_config)), output_stream) {
-    _link_legacy_forcing();
+//) : Catchment_Formulation(std::move(id), std::move(std::make_unique<Forcing>(forcing_config)), output_stream) {
+) : Catchment_Formulation(std::move(id), std::move(std::make_unique<CsvPerFeatureForcingProvider>(forcing_config)), output_stream) {


I would tend to agree...

hellkite500 · 2022-09-12T14:20:19Z

src/realizations/catchment/Tshirt_C_Realization.cpp

+    }
+    time_t start_time = this->forcing->get_data_start_time();
+    time_t stop_time = this->forcing->get_data_stop_time();
+    time_t t_current = start_time + t_index * t_delta_s;


again, the relationship between start_time, t_index, and t_delta_s is not super clear

hellkite500 · 2022-09-12T14:24:06Z

src/realizations/catchment/LSTM_Realization.cpp

- * @param longitude
- * @param area_square_km
- */
-LSTM_Realization::LSTM_Realization(


Also, any reason this constructor got removed?

It was not used anywhere. It could be re-added with forcing_params changed to a DataProvider, but it would just be zombie code.

The intent of it was to ensure that if someone wanted to use the realization for something they could create it directly with the paths (it is a convenience constructor). It wasn't exactly meant to be used in our own code.

My guess it it was using a forcing config object, not sure why it was removed an not refactored though.

Somewhat subjective, but it's unused in the application... this isn't a general purpose library (at least not yet)... so it's arguably goldplating. Just more code to maintain.

Nuke it if you want...we have moved away in large part from the development kit style of implementing realizations, relying instead on BMI for the moment. I would, however, suggest that that deprecation/removal be done in its own isolated commit so that a history/blame on the file in the future would show a clean way to revive that constructor.

donaldwj

Nel's concerns in the LSTM code need to be addressed, otherwise no issues that I see

stcui007 · 2022-10-11T09:29:07Z

Will test it out.

…

On Fri, Aug 12, 2022 at 3:43 PM Matt Williamson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/realizations/catchment/Formulation_Constructors.hpp <#432 (comment)>: > @@ -18,7 +18,7 @@ #include "Bmi_Multi_Formulation.hpp" #include "Bmi_Py_Formulation.hpp" #include <GenericDataProvider.hpp> -#include "CsvPerFeatureForcingProvider.hpp" +//#include "CsvPerFeatureForcingProvider.hpp" You will need #include "CsvPerFeatureForcingProvider.hpp" in files where CsvPerFeatureForcingProvider is explicitly constructed... for the moment that means the two tshirt files also (though ideally those dependencies can be removed, per the discussion there). — Reply to this email directly, view it on GitHub <#432 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRKUXK67F7GYXVU7USTVY2ZNNANCNFSM56JJ3L4Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

stcui007 requested review from hellkite500, donaldwj and mattw-nws August 11, 2022 19:30

mattw-nws requested changes Aug 11, 2022

View reviewed changes

mattw-nws reviewed Aug 12, 2022

View reviewed changes

mattw-nws reviewed Aug 18, 2022

View reviewed changes

mattw-nws mentioned this pull request Aug 26, 2022

Fix NetCDFPerFeatureDataProvider returning the same forcing for all catchments #437

Merged

11 tasks

mattw-nws changed the title ~~Use new forcing~~ Remove deprecated Forcing class dependency from legacy formulations Aug 26, 2022

stcui007 and others added 5 commits September 8, 2022 16:23

Initial changes to use CsvPerFeatureForcingProvider

0ad6853

Remove the old legacy forcing

c158f9c

Update and refactoring to use new forcing provider

2b851a3

Retrofits LSTM_Realization to use GenericDataProvider.

2d44340

Adjust test value... forcing may have changed, as the path moved at l…

6140c52

…east.

mattw-nws force-pushed the use_new_forcing branch from 9038f79 to 6140c52 Compare September 8, 2022 16:31

Matt Williamson added 2 commits September 8, 2022 17:26

rollback merge mistake

6d87c02

Finish the job.

f284f90

mattw-nws approved these changes Sep 8, 2022

View reviewed changes

mattw-nws mentioned this pull request Sep 9, 2022

Fix legacy formulations to pass catchment ID to CatchmentAggrDataSelector #439

Closed

hellkite500 reviewed Sep 12, 2022

View reviewed changes

donaldwj requested changes Sep 12, 2022

View reviewed changes

mattw-nws mentioned this pull request Sep 15, 2022

Refactor and remove deprecated ForcingProvider functions #442

Closed

hellkite500 requested a review from donaldwj September 21, 2022 13:56

donaldwj approved these changes Sep 21, 2022

View reviewed changes

hellkite500 merged commit aca6450 into NOAA-OWP:master Sep 21, 2022

Remove deprecated Forcing class dependency from legacy formulations #432

Remove deprecated Forcing class dependency from legacy formulations #432

Conversation

stcui007 commented Aug 11, 2022

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Testing checklist (automated report can be put here)

Target Environment support

mattw-nws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stcui007 commented Aug 24, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

mattw-nws Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldwj left a comment

Choose a reason for hiding this comment

stcui007 commented Oct 11, 2022 via email

mattw-nws Aug 12, 2022 •

edited

Loading

mattw-nws Sep 12, 2022 •

edited

Loading

mattw-nws Sep 12, 2022 •

edited

Loading