Null/Constant Forcing Provider #601
Conversation
| * @return Whether the param's value is an aggregate sum. | ||
| */ | ||
| //TODO this function doesn't seem being used | ||
| inline bool is_param_sum_over_time_step(const std::string& name) { |
There was a problem hiding this comment.
This method should probably throw also, since there are no variables... certainly none of the variables listed below here will be provided.
There was a problem hiding this comment.
It looks like it. Will test it.
There was a problem hiding this comment.
Actually, on review... this function should probably not exist. Move the exception to is_property_sum_over_time_step directly (this is the method actually in the DataProvider interface) and delete is_param_sum_over_time_step (and its documentation comment above).
| * @param name The name of the forcing param for which the current value is desired. | ||
| * @return Whether the param's value is an aggregate sum. | ||
| */ | ||
| //TODO this function doesn't seem being used |
There was a problem hiding this comment.
Yes, this function should never get called if there are no variables to call it on, for sure. I think it gets called for other providers? But it could be put in for future capability. In either case, as mentioned below, in this provider it should probably throw an exception.
There was a problem hiding this comment.
Will test. Logically it should not be used on forcing variables retrieved via BMI.
| * @param file_name Forcing file name | ||
| */ | ||
| // No file to read | ||
| void read_csv() |
There was a problem hiding this comment.
I would think this method should be entirely removed. Is there some reason we need to add a "precip" variable? Is there a need to have time in available_forcings? Unless it breaks things, the idea was that available_forcings (or, rather, whatever is returned from get_available_variable_names) should be empty.
There was a problem hiding this comment.
I tried to remove it entirely. but that cuased a problem. Somehow we need at least one available forcing variable names to the Formulation_Manager to retrieve the start and stop time. Otherwise, it causes a runtime error.
There was a problem hiding this comment.
Interesting. So, if this is the case, then we might need to move to the ZerosDataProvider as discussed in #569. However, I still wonder if this can be avoided. I think I know where that is coming from, let me try to find it.
| std::string var_name = "precip"; | ||
| std::string units = ""; | ||
| auto wkf = data_access::WellKnownFields.find(var_name); | ||
| if(wkf != data_access::WellKnownFields.end()){ |
There was a problem hiding this comment.
Actually, are either time or precip found in WellKnownFields? I think not (AorcForcing.hpp)... so available_forcings probably is empty and this whole block is doing nothing. Right?
There was a problem hiding this comment.
No, by precip I meant to use precip_rate. No sure if time is needed. I am trying to mimic a forcing file with some variable names here.
There was a problem hiding this comment.
So, if this provider returns any variables, it should not return one of the standard/AORC ones, because that will just interfere with (or at least confuse) getting the same variables from a BMI forcing provider. If we have to return some variable, it should be named zero or something as discussed in #569 .
There was a problem hiding this comment.
You are right. This function now has only one line of codes.
| } | ||
|
|
||
| std::vector<std::string> available_forcings; | ||
| std::unordered_map<std::string, std::string> available_forcings_units; |
There was a problem hiding this comment.
If read_csv is removed then this variable will be unused and should be removed.
There was a problem hiding this comment.
We cannot remove read_csv as it stands now. May be a better name is needed. It seems some non-empty string variable is needed in the initialization process.
| * @param index The index of the desired forcing time step from which to obtain the value. | ||
| * @return The particular param's value at the given forcing time step. | ||
| */ | ||
| inline double get_value_for_param_name(const std::string& name, int index) { |
There was a problem hiding this comment.
This looks like it was only used internally in CsvPerFeatureForcingProvider and isn't used in this class, so should be removed.
| std::vector<time_t> time_epoch_vector; | ||
| int forcing_vector_index; | ||
|
|
||
| /// \todo: Are these used? |
There was a problem hiding this comment.
Indeed, there are several instance variables here that are no longer used and should be removed.
| @@ -302,12 +302,15 @@ namespace realization { | |||
|
|
|||
| time_t get_variable_time_begin(const std::string &variable_name) { | |||
| std::string var_name = variable_name; | |||
| //std::cout << "availableData.size(): = " << availableData.size() << std::endl; | |||
There was a problem hiding this comment.
Remove all commented-out debug prints from committed code.
| long record_duration() override { | ||
| //return time_epoch_vector[1] - time_epoch_vector[0]; | ||
| //TODO find a more general way to set it | ||
| long timestep_size = 3600; |
There was a problem hiding this comment.
This should probably be 1 or even 0? It may not matter, but this provider doesn't really have a timestep.
There was a problem hiding this comment.
I'll probably use 1 but will test 0 to see what it does.
|
Replied to your comments on github.
…On Mon, Aug 7, 2023 at 9:52 AM Matt Williamson ***@***.***> wrote:
***@***.**** requested changes on this pull request.
There are some things to work on here... I will look into the question
about the path key a little later.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> +
+ /**
+ * Get whether a param's value is an aggregate sum over the entire time step.
+ *
+ * Certain params, like rain fall, are aggregated sums over an entire time step. Others, such as pressure, are not
+ * such sums and instead something else like an instantaneous reading or an average value over the time step.
+ *
+ * It may be the case that forcing data is needed for some discretization different than the forcing time step.
+ * These values can be calculated (or at least approximated), but doing so requires knowing which values are summed
+ * versus not.
+ *
+ * @param name The name of the forcing param for which the current value is desired.
+ * @return Whether the param's value is an aggregate sum.
+ */
+ //TODO this function doesn't seem being used
+ inline bool is_param_sum_over_time_step(const std::string& name) {
This method should probably throw also, since there are no variables...
certainly none of the variables listed below here will be provided.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> +
+
+ /**
+ * Get whether a param's value is an aggregate sum over the entire time step.
+ *
+ * Certain params, like rain fall, are aggregated sums over an entire time step. Others, such as pressure, are not
+ * such sums and instead something else like an instantaneous reading or an average value over the time step.
+ *
+ * It may be the case that forcing data is needed for some discretization different than the forcing time step.
+ * These values can be calculated (or at least approximated), but doing so requires knowing which values are summed
+ * versus not.
+ *
+ * @param name The name of the forcing param for which the current value is desired.
+ * @return Whether the param's value is an aggregate sum.
+ */
+ //TODO this function doesn't seem being used
Yes, this function should never get called if there are no variables to
call it on, for sure. I *think* it gets called for other providers? But
it could be put in for future capability. In either case, as mentioned
below, in this provider it should probably throw an exception.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> +
+ if (forcing_vectors.count(can_name) > 0) {
+ return forcing_vectors[can_name].at(index);
+ }
+ else {
+ throw std::runtime_error("Cannot get forcing value for unrecognized parameter name '" + name + "'.");
+ }
+ }
+
+ /**
+ * @brief Read Forcing Data from CSV
+ * Reads only data within the specified model start and end date-times.
+ * @param file_name Forcing file name
+ */
+ // No file to read
+ void read_csv()
I would think this method should be entirely removed. Is there some reason
we need to add a "precip" variable? Is there a need to have time in
available_forcings? Unless it breaks things, the idea was that
available_forcings (or, rather, whatever is returned from
get_available_variable_names) should be empty.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + }
+
+ /**
+ * @brief Read Forcing Data from CSV
+ * Reads only data within the specified model start and end date-times.
+ * @param file_name Forcing file name
+ */
+ // No file to read
+ void read_csv()
+ {
+ //std::string var_name = CSDMS_STD_NAME_RAIN_VOLUME_FLUX;
+ available_forcings.push_back("time");
+ std::string var_name = "precip";
+ std::string units = "";
+ auto wkf = data_access::WellKnownFields.find(var_name);
+ if(wkf != data_access::WellKnownFields.end()){
Actually, are either time or precip found in WellKnownFields? I think not
(AorcForcing.hpp)... so available_forcings probably *is* empty and this
whole block is doing nothing. Right?
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + {
+ //std::string var_name = CSDMS_STD_NAME_RAIN_VOLUME_FLUX;
+ available_forcings.push_back("time");
+ std::string var_name = "precip";
+ std::string units = "";
+ auto wkf = data_access::WellKnownFields.find(var_name);
+ if(wkf != data_access::WellKnownFields.end()){
+ units = units.empty() ? std::get<1>(wkf->second) : units;
+ available_forcings.push_back(var_name); // Allow lookup by non-canonical name
+ available_forcings_units[var_name] = units; // Allow lookup of units by non-canonical name
+ var_name = std::get<0>(wkf->second); // Use the CSDMS name from here on
+ }
+ }
+
+ std::vector<std::string> available_forcings;
+ std::unordered_map<std::string, std::string> available_forcings_units;
If read_csv is removed then this variable will be unused and should be
removed.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + //TODO this one used in Bmi_Module_Formulation.hpp and Bmi_Multi_Formulation.hpp
+ inline bool is_property_sum_over_time_step(const std::string& name) override {
+ return is_param_sum_over_time_step(name);
+ }
+
+ const std::vector<std::string> &get_available_variable_names() override {
+ return available_forcings;
+ }
+
+ private:
+
+ /**
+ * @brief Checks forcing vector index bounds and adjusts index if out of vector bounds
+ * /// \todo: Bounds checking is based on precipitation vector. Consider potential for vectors of different sizes and indices.
+ */
+ inline void check_forcing_vector_index_bounds()
Looks like this was dead code in CsvPerFeatureForcingProvider, sorry. It
should be removed.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + forcing_vector_index = time_epoch_vector.size() - 1;
+ /// \todo: Return appropriate warning
+ std::cout << "WARNING: Reached beyond the size of the forcing vector. Therefore, setting index to last value of the vector." << std::endl;
+ }
+
+ return;
+ }
+
+ /**
+ * Get the current value of a forcing param identified by its name.
+ *
+ * @param name The name of the forcing param for which the current value is desired.
+ * @param index The index of the desired forcing time step from which to obtain the value.
+ * @return The particular param's value at the given forcing time step.
+ */
+ inline double get_value_for_param_name(const std::string& name, int index) {
This looks like it was only used internally in
CsvPerFeatureForcingProvider and isn't used in this class, so should be
removed.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + available_forcings_units[var_name] = units; // Allow lookup of units by non-canonical name
+ var_name = std::get<0>(wkf->second); // Use the CSDMS name from here on
+ }
+ }
+
+ std::vector<std::string> available_forcings;
+ std::unordered_map<std::string, std::string> available_forcings_units;
+
+ /// \todo: Look into aggregation of data, relevant libraries, and storing frequency information
+ std::unordered_map<std::string, std::vector<double>> forcing_vectors;
+
+ /// \todo: Consider making epoch time the iterator
+ std::vector<time_t> time_epoch_vector;
+ int forcing_vector_index;
+
+ /// \todo: Are these used?
Indeed, there are several instance variables here that are no longer used
and should be removed.
------------------------------
In include/realizations/catchment/Bmi_Multi_Formulation.hpp
<#601 (comment)>:
> @@ -302,12 +302,15 @@ namespace realization {
time_t get_variable_time_begin(const std::string &variable_name) {
std::string var_name = variable_name;
+ //std::cout << "availableData.size(): = " << availableData.size() << std::endl;
Remove all commented-out debug prints from committed code.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + end_date_time_epoch(forcing_config.simulation_end_t),
+ current_date_time_epoch(forcing_config.simulation_start_t),
+ forcing_vector_index(-1)
+ {
+ read_csv();
+ }
+
+ // BEGIN DataProvider interface methods
+
+ /**
+ * @brief the inclusive beginning of the period of time over which this instance can provide data for this forcing.
+ *
+ * @return The inclusive beginning of the period of time over which this instance can provide this data.
+ */
+ long get_data_start_time() override {
+ //FIXME: Trace this back and you will find that it is the simulation start time, not having anything to do with the forcing at all.
Ugh. I am reminded of this. It may not matter in the case of
NullForcingProvider... it also may only matter for legacy formulations...
we'll have to see. No action here yet, lets see what it does with the
values you have.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + * @return The exclusive ending of the period of time over which this instance can provide this data.
+ */
+ long get_data_stop_time() override {
+ //return end_date_time_epoch;
+ return LONG_MAX;
+ }
+
+ /**
+ * @brief the duration of one record of this forcing source
+ *
+ * @return The duration of one record of this forcing source
+ */
+ long record_duration() override {
+ //return time_epoch_vector[1] - time_epoch_vector[0];
+ //TODO find a more general way to set it
+ long timestep_size = 3600;
This should probably be 1 or even 0? It may not matter, but this provider
doesn't really *have* a timestep.
—
Reply to this email directly, view it on GitHub
<#601 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SROZG6CQLU3CTGDGEILXUD6K5ANCNFSM6AAAAAA3EJE6NQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Now, I made it just store a "dummy" variable in available_forcings.
…On Mon, Aug 7, 2023 at 12:11 PM Matt Williamson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> + }
+
+ /**
+ * @brief Read Forcing Data from CSV
+ * Reads only data within the specified model start and end date-times.
+ * @param file_name Forcing file name
+ */
+ // No file to read
+ void read_csv()
+ {
+ //std::string var_name = CSDMS_STD_NAME_RAIN_VOLUME_FLUX;
+ available_forcings.push_back("time");
+ std::string var_name = "precip";
+ std::string units = "";
+ auto wkf = data_access::WellKnownFields.find(var_name);
+ if(wkf != data_access::WellKnownFields.end()){
So, if this provider returns *any* variables, it should *not* return one
of the standard/AORC ones, because that will just interfere with (or at
least confuse) getting the same variables from a BMI forcing provider. If
we have to return some variable, it should be named zero or something as
discussed in #569 <#569> .
—
Reply to this email directly, view it on GitHub
<#601 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRLNE74DYTNUXMZCH4TXUEOUNANCNFSM6AAAAAA3EJE6NQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Will do.
…On Tue, Aug 8, 2023 at 2:07 PM Matt Williamson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/forcing/NullForcingProvider.hpp
<#601 (comment)>:
> +
+ /**
+ * Get whether a param's value is an aggregate sum over the entire time step.
+ *
+ * Certain params, like rain fall, are aggregated sums over an entire time step. Others, such as pressure, are not
+ * such sums and instead something else like an instantaneous reading or an average value over the time step.
+ *
+ * It may be the case that forcing data is needed for some discretization different than the forcing time step.
+ * These values can be calculated (or at least approximated), but doing so requires knowing which values are summed
+ * versus not.
+ *
+ * @param name The name of the forcing param for which the current value is desired.
+ * @return Whether the param's value is an aggregate sum.
+ */
+ //TODO this function doesn't seem being used
+ inline bool is_param_sum_over_time_step(const std::string& name) {
Actually, on review... this function should probably not exist. Move the
exception to is_property_sum_over_time_step directly (this is the method
actually in the DataProvider interface) and delete
is_param_sum_over_time_step (and its documentation comment above).
—
Reply to this email directly, view it on GitHub
<#601 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRIZPGH3XZVUR45IUX3XUKFAJANCNFSM6AAAAAA3EJE6NQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I merged #624 ... I think this will eliminate the failure that occurs when there are no variables in the NullForcingProvider. Can you rebase on master now and try removing the dummy variable? |
|
Yes. I'll try it.
…On Tue, Sep 5, 2023 at 10:10 AM Matt Williamson ***@***.***> wrote:
I merged #624 <#624> ... I think
this will eliminate the failure that occurs when there are *no* variables
in the NullForcingProvider. Can you rebase on master now and try removing
the dummy variable?
—
Reply to this email directly, view it on GitHub
<#601 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRLEYAPXPAXDTNODSQLXY46HTANCNFSM6AAAAAA3EJE6NQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
stcui007
force-pushed
the
null_forcing_provider
branch
from
abc3b9e to
24bb61b
Compare
| #include <UnitsHelper.hpp> | ||
|
|
||
| /** | ||
| * @brief Forcing class providing time-series precipiation forcing data to the model. |
There was a problem hiding this comment.
Update this doc string to reflect that this forcing provider does not, in fact, provide any forcing data!
|
I have changed the status of draft PR to PR.
…On Fri, Sep 8, 2023 at 12:13 PM Matt Williamson ***@***.***> wrote:
Assigned #601 <#601> to @stcui007
<https://github.com/stcui007>.
—
Reply to this email directly, view it on GitHub
<#601 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRONZMUJM3SGZXA5MDTXZNG2VANCNFSM6AAAAAA3EJE6NQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
I want to try to prove that this works with a more real-world case--especially the get_data_begin_time/get_data_end_time return values. I'm getting with @hellkite500 to see if we can try this with the latest forcing modules from that workstream... meanwhile if you get stuck on other things have a look at these comments.
| @@ -285,6 +285,7 @@ namespace realization { | |||
| for(std::map<std::string,std::shared_ptr<data_access::GenericDataProvider>>::iterator iter = availableData.begin(); iter != availableData.end(); ++iter) | |||
| { | |||
| var_name = iter->first; | |||
| //std::cout << "availableData: var_name = " << var_name << std::endl; | |||
There was a problem hiding this comment.
Actually, this function in Bmi_Multi_Formulation almost certainly needs the same change as was done in #624 for the NullForcingProvider to not break things. Once this provider gets used, if Bmi_Multi_Formulation::get_variable_time_end is called, it will probably throw on line 302 here because no forcing variables will be found to return an end time.
There was a problem hiding this comment.
Yeah, agree.
There is also the same codes in record_duration() function.
There was a problem hiding this comment.
At some point, I thought I deleted many of the header lines. Don't know why they are still there. Will do.
|
Ditto my request on your earlier PR - please actually describe the changes, and fix the formatting of the PR description Will work on it. |
|
I added two minor commits, one just adjusts the example realization config to only use features already used in other realization configs (e.g. SLoTH, CFE, Noah), and the second includes an easy change to make the |
These looks fine to me. The addition of another realization configuration is useful. |
mattw-nws
force-pushed
the
null_forcing_provider
branch
from
2a9fa2e to
04853cf
Compare
A "forcing" provider that provides no data. This--primarily--is such that a BMI module can act as the forcing provider, providing time-varying input data from any source that can be packaged and provided in a module.
Additions
Removals
Changes
"path"parameter in forcing section of realization config optional, as this is not required for NullForcingProvider.Testing
Screenshots
Notes
Todos
"path"is optional for at least one provider now.Checklist
Testing checklist (automated report can be put here)
Target Environment support