Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Utils: extend Create/RecreateArray functions #113

Closed
PMeira opened this issue Jul 10, 2022 · 1 comment
Closed

API Utils: extend Create/RecreateArray functions #113

PMeira opened this issue Jul 10, 2022 · 1 comment

Comments

@PMeira
Copy link
Member

PMeira commented Jul 10, 2022

To handle matrix sizes directly without extra parameters and with good backwards compatibility, cnt: PAPISize will be adjusted.

  • Before v0.10, this was a pointer to a single element.
  • For v0.10, we added a second element used as the capacity of the current buffer, reducing allocations.
  • For v0.12.1, we can add 2 more elements to represent the matrix shape (num rows, num cols). To keep compatibility, we can add a flag to avoid accessing those elements in DSS C-API. If the flag is not set, we fill the expected matrix shape.

Here, we will have to review the CAPI_* files to pass the matrix shape to the reallocation functions. As a bonus, these files can then be post-processed to extract some information for dss-extensions/dss-extensions#6. The new Obj_* functions could be handled too, if it's not too much work.

On the other projects, support can be added by toggling the behavior and checking the values returned. For the end-user, with the new behavior turned off, plain vectors would still be returned, so compatibility is kept. When enabled, if the last element is non-zero, a matrix can be return instead of a vector.

For Python, MATLAB and Julia, I see no obvious issues with this. For C#, we wouldn't support right now since there is no matrix support at the moment. For C++, some template specializations or if constexprs should do the trick.

The main changes would be restricted to the functions that handle the array types. In Python, looks like it would be a single conditional:

    def get_float64_array(self, func, *args):
        ptr = self.ffi.new('double**')
        cnt = self.ffi.new('int32_t[4]') # CFFI zero-inits this
        func(ptr, cnt, *args)
        res = np.frombuffer(self.ffi.buffer(ptr[0], cnt[0] * 8), dtype=np.float64).copy()
        self.lib.DSS_Dispose_PDouble(ptr)
        if cnt[3] != 0: # if in compat mode, or it's actually a vector, value would be 0
            return res.reshape((cnt[2], cnt[3]))
            
        return res
        
    def get_float64_gr_array(self):
        ptr, cnt = self.gr_float64_pointers
        if cnt[3] != 0: # if in compat mode, or it's actually a vector, value would be 0
            return np.frombuffer(self.ffi.buffer(ptr[0], cnt[0] * 8), dtype=np.float64).copy().reshape((cnt[2], cnt[3]))

        return np.frombuffer(self.ffi.buffer(ptr[0], cnt[0] * 8), dtype=np.float64).copy()

Finally, we could consider some way of passing a custom "allocator" function to wrap a few things. For instance, in non-optimized general code, there is still a potential extra copy right now to setup the final container. If we can use a setup function in DSS C-API to init the final container (NumPy array, MATLAB array, Eigen::Matrix<>...), no extra copies would be required. This could be beneficial.

PMeira added a commit that referenced this issue Jul 10, 2022
The matrix shape is not filled yet, but we will use the same memory layout later.

Ref #113
PMeira added a commit to dss-extensions/DSS-Python that referenced this issue Jul 11, 2022
@PMeira
Copy link
Member Author

PMeira commented Mar 23, 2023

Done, merging tomorrow.

@PMeira PMeira closed this as completed Mar 23, 2023
PMeira added a commit that referenced this issue Mar 23, 2023
Reminder: the docs still need to be updated to reflect the change.

Closes #113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant