Fix NetCDFPerFeatureDataProvider returning the same forcing for all catchments #437
Conversation
…s... or maybe TrivialForcingProvider could be used?
mattw-nws
changed the title
There was a problem hiding this comment.
I noticed a few small things that I added inline review comments for, but nothing imperative.
I also noticed that the Simple_Lumped_Model_Realization::get_response function still makes some calls to the selector's constructor while passing in "" for id. Strictly speaking this looks like it could cause a problem, although it also seems reasonably safe to defer fixing it, given the class involved, as long as it gets tracked.
@mattw-nws I'll go ahead and approve, but let me know if you want to adjust any of those minor items and I'll come back to look at this again.
| @@ -1118,7 +1118,7 @@ namespace realization { | |||
| varItemSize); | |||
| if (varItemSize != get_bmi_model()->GetVarNbytes(var_name)) { | |||
| //more than a single value needed for var_name | |||
| auto values = provider->get_values(CatchmentAggrDataSelector("",var_map_alias, model_epoch_time, t_delta, | |||
| auto values = provider->get_values(CatchmentAggrDataSelector(this->get_catchment_id(),var_map_alias, model_epoch_time, t_delta, | |||
There was a problem hiding this comment.
At the risk of being a bit pedantic: the change probably shouldn't keep the bad spacing between the first two args.
I'm not going to keep pointing it out, but I did notice it in a few other places also.
| @@ -644,7 +644,7 @@ namespace realization { | |||
| std::shared_ptr <data_access::GenericDataProvider> nested_module = | |||
| std::dynamic_pointer_cast<data_access::GenericDataProvider>(data_provider_iter->second); | |||
| long nested_module_time = nested_module->get_data_start_time() + ( this->get_model_current_time() - this->get_model_start_time() ); | |||
| auto selector = CatchmentAggrDataSelector("",var_name,nested_module_time,this->record_duration(),"1"); | |||
| auto selector = CatchmentAggrDataSelector(this->get_catchment_id(),var_name,nested_module_time,this->record_duration(),"1"); | |||
There was a problem hiding this comment.
I am going to point out this exception, because the arg spacing here is already consistent, at least internally.
| // Assume the catchment ID is equal to or embedded in the formulation `id` | ||
| size_t idx = id.find("."); | ||
| set_catchment_id( idx == std::string::npos ? id : id.substr(0, idx) ); | ||
| }; |
There was a problem hiding this comment.
A couple things I notice, but that are by no means deal-breakers.
First, we may get slightly better performance using the find that accepts a char; e.g., size_t idx = id.find('.');.
Second, the code duplication is bugging me. No straightforward way of eliminating it entirely, but it might be better to move the parsing and substring logic here to set_catchment_id directly. I can't think of a reason why it makes sense to apply it at construction but not other times set_catchment_id is called.
There was a problem hiding this comment.
Yeah, this bugged me a little... I'm willing to duplicate something (in this length/complexity neighborhood) twice but not three times. I did not want to put it in the getter... arguably, it shouldn't be in the setter because that should receive something that is already a catchment ID, while this is a formulation ID... this code (functionally speaking) translates a formulation ID into a catchment ID. So... yeah short of breaking it into a utility function (probably overkill for something this small) I don't see a great answer.
Yes, this occurred to me, the two t-shirt models probably have the same. I was similarly on the fence about it... since you also identified it that is maybe tipping me off the fence toward resolving it. Or at least yeah I should file an issue for the remaining instances. Those formulations wouldn't work with NetCDFPerFeatureDataProvider... which is kindof the point of #432, for instance. |
|
Okay, decided to add #439 to address legacy formulations. |
The catchment ID was not being passed through to the selector. Rectified.
Additions
Catchment_Formulation::get_catchment_id()be passed to selector constructors inBmi_Module_FormulationandBmi_Multi_FormulationRemovals
Changes
get_catchment_id()trim any.Nsuffix from the realization IDs in the case of nested formulations.Testing
get_catchment_id()... some more work tests would probably be good for the whole pipeline, but that's either an integration test or would require more mocks.Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support