PowerGridModel / power-grid-model

Python/C++ library for distribution power system analysis
Mozilla Public License 2.0
143 stars 30 forks source link

[FEATURE] Explicitly disallow unsupported data types (undefined behavior) #774

Open mgovers opened 4 days ago

mgovers commented 4 days ago

Describe the feature request

Explicitly go from undefined behavior to disallowed behavior when unsupported data formats are provided to the user.

Background

The power-grid-model only supports specific data types for the input, output and update datasets. This is true for both the Python API and the C API, and for both row-based and columnar data. The initialize_array and power_grid_meta_data (undocumented)

Providing other data types results in undefined behavior (UB) for both the Python API and the C API.

However, the unknowing Python user may not realize that they are doing stuff that is not officially supported, because the current implementation happens to be a copy (because we use the as_contiguous_array to get the raw buffers). Even worse: the copy also implicitly casts data to their specified supported dtype, potentially losing or even incorrectly truncating data. This erroneous behavior should be explicitly disallowed

NOTE: THIS FUNCTIONALITY WAS NEVER OFFICIALLY SUPPORTED.

Out of scope

If the user provides a dataset type that is equal (==) but not equivalent (is) to the power_grid_meta_data dtype, then a copy is done. While not recommended for performance reasons, this remains supported behavior.

TODO

Since this is potentially breaking workflows of users that are incorrectly using the power grid model, we should follow a 2-step approach: first deprecating the functionality, and then explicitly disallowing it in

Step 1: deprecation

Step 2: removal

TonyXiang8787 commented 4 days ago

I thought we agreed to immediately go to step 2. Is that correct? No warnings. Just in the next minor version, a hard error.

mgovers commented 4 days ago

I thought we agreed to immediately go to step 2. Is that correct? No warnings. Just in the next minor version, a hard error.

@nitbharambe correctly mentioned that some people might not use it correctly now. they want to be notified beforehand so that they have time to set to power-grid-model>=1.9,<1.10 / power-grid-model = "^1.9" before it actually breaks. Even if it's only a 2-week notice, we should use any means to ease our users' life.

nitbharambe commented 4 days ago

This and it also helps us separate this task into implementation and version bump.

I thought we agreed to immediately go to step 2. Is that correct? No warnings. Just in the next minor version, a hard error.

@nitbharambe correctly mentioned that some people might not use it correctly now. they want to be notified beforehand so that they have time to set to power-grid-model>=1.9,<1.10 / power-grid-model = "^1.9" before it actually breaks. Even if it's only a 2-week notice, we should use any means to ease our users' life.