Bmi array support

[hellkite500]

This PR allows a BMI module to provide an array of values as a single 1D output which can then be passed/consumed by another BMI module in a Multi BMI configuration.

Additions

• get_values function added to ForcingProvider, with a default implementation
• BMI Module Formulation custom override of ForcingProvider::get_values
• get_converted_values function added to unit helper for unit conversion of an array of values
• BMI utilities module with function to cast and copy values based on string type tags
• Test module for testing setting/getting array of values across two C++ BMI modules

Removals

• Deprecated templated GetValue( std::string & ) overload from BMI adapters. Functionality moved to BMI utilities free function.

Changes

• check_internal_providers now returns a vector of values instead of a single value
• add new array variable to c++ bmi test model
• updated implementation set_model_inputs_prior_to_update which checks for array like values and handles them accordingly.

Testing

1. New functionality tested, all existing tests passing locally

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
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

Target Environment support

• Linux
• MacOS

[donaldwj]

The unit conversion code will leak memory on exceptions. As we don't always treat that as fatal error this needs to be addressed. The other comments are just places where I would like design choices explained.

[donaldwj]

Do we want this to work on raw arrays. As defined this will change random memory if 'count' is not correctly set to the size of the array. Also returning a value is misleading as we are mutating the call argument.

[mattw-nws]

Since this is designed to work with BMI array inputs and outputs, I imagine a version to work on raw arrays is wanted. We face the same risk of count being incorrect in all those BMI calls, so there's little lost here.

However... I didn't realize we were mutating the input... so if we used this on the result of get_value_ptr()...that could be bad. Ideally I'd say this should not mutate, or that another function (e.g. static void convert_values_in_place(...)) would be better to have for the use case where we explicitly want to do that. Unsure what the mem mgmt semantics would be in C style if we return a copy... malloc and assume/document that the caller is now responsible for freeing? Require the caller to allocate and pass a new buffer as an additional parameter?

[hellkite500]

This is just a thin wrapper around the UDUNITS function that works on arrays cv_convert_doubles(conv, values, count, values);

double* UnitsHelper::get_converted_values(const std::string &in_units, double* values, const std::string &out_units, const size_t& count)
{
    if(in_units == out_units){
        return values; // Early-out optimization
    }
    std::call_once(unit_system_inited, init_unit_system);
    ut_unit* to = NULL;
    ut_unit* from = NULL;
    cv_converter* conv = get_converter(in_units, out_units, to, from);

    cv_convert_doubles(conv, values, count, values);
    ut_free(from);
    ut_free(to);
    cv_free(conv);
    return values;
}

And returning the value (reference to the mutated input variable) is just syntactic convenience and follows the potentially expected pattern as the regular get_converted_value which returns the converted value. It could be said that NOT returning it is misleading in that context. I implemented it this way to be flexible, and it is a somewhat common pattern when working with mutable args behind a pointer.

[donaldwj]

Why create a new selector, this appears identical to the input selector?

[mattw-nws]

Good question... I think this is essentially copy-pasted from the get_value implementation above, which does the same... possibly an artifact of an original intent to ensure that a BmiDataSelector was used, but as discussed there's a slight performance benefit to doing it this way. Not sure what's "best" for both these cases... in theory, an actual CatchmentAggrDataSelector could get passed in with a non-empty catchment id... this code ensures that there is not one downstream....that is one difference, but I'm not sure if that's a good thing or bad thing now, on reflection.

[hellkite500]

@mattw-nws any thoughts, I believe this was one of your additions.

[donaldwj]

Well that would have been the result of least change policy when adding data selectors, In any case there is not reason to do that.

[donaldwj]

maybe attach smart pointer to this to make sure that memory is not leaked in the case of an exception. We have in general avoided new/delete malloc/free as much as possible.

[hellkite500]

This is partly due to getting a smart pointer of the correct size which also properly deletes the correct amount of memory. A void* of the correct size is required by BMI to populate the array in GetValue. I recall trying to do this with a smart pointer and having issues with it, but I can't remember off the top of my head exactly what the complication was. It very well may have been getting it typed as void* with the correct number of bytes allocated.

The use of manual allocation/cleanup here is at least isolated to this function, and we can fix the potential leak in the exception throwing. If we can do this sufficiently with a smart pointer, we can change this.

[mattw-nws]

Interesting question. Maybe you ran into a problem using unique_ptr<void>? Maybe I'll play with this.

[donaldwj]

This fixes the memory link anytime there was an error, which was my only concern with the original request.

[donaldwj]

At this position we will get a memory leak on the above exception.

[hellkite500]

Pretty sure this is a fatal error at this point, but in case it is caught elsewhere, we could delete before we throw as well.

[donaldwj]

I would add the delete[] before the throw, no reason to leave a potential leak, even on something assumed to be fatal, if we dont need to.

[mattw-nws]

I'm good with it at this point. @donaldwj see if your change requests have been addressed.

[donaldwj]

This fixes the memory link anytime there was an error, which was my only concern with the original request.