NOAA-OWP / lstm

Other
12 stars 17 forks source link

Ensure correct implementations for getter and setter functions #24

Closed robertbartel closed 2 years ago

robertbartel commented 2 years ago

There are some issues with the current implementations of the getter and setter functions. These are things that don't properly adhere to the BMI specification.

get_value

First, the signature of this function isn't correct. Right now it is:

def get_value(self, var_name):

But in the specification it is defined as:

def get_value(self, name: str, dest: np.ndarray) -> np.ndarray:

This directly prevents it from behaving correctly. It is expected to copy the values of the desired variable into the (already appropriately initialized) dest NumPy array.

Also related, the function right now is returning the backing NumPy array from get_value_ptr. It probably needs to return dest once that's added. Regardless, it shouldn't return a direct reference to the backing variable.

get_value_at_indices

This will need to be modified after changes to get_value. I think it's also making an unnecessary copy of the entire backing NumPy array, before then copying the right individual values into the supplied dest array.

set_value

This function has its required second parameter for containing the values to set, but appears to makes invalid assumptions that are based on something beyond the BMI specification: e.g., that it is a singleton array. That in particular also seems to be contradicted by there being valid get_value_at_indices and set_value_at_indices functions (i.e., why not raise an exception to indicate there is only index 0, and thus the at_indices functions are unnecessary and inappropriate?).

set_value_at_indices

Again, this seems to try to account for the possibility of singleton arrays in ways that contradict needing at_indices functions, and that aren't strictly consistent with BMI.

robertbartel commented 2 years ago

Note that this assessment assumes the inclusion of #23.

madMatchstick commented 2 years ago

Be aware that this change effects the following bmi functions,

  get_var_itemsize()
  get_value()
  get_value_ptr()
  set_value_at_indices()
  get_value_at_indices()

Any build/run scripts (py or ipynb) would also need to be updated for these five function calls.\

Useful links related to post: