Array sizes in GrB_Matrix_export

mcmillan03 commented 2 years ago

In the original 'export' method -- GrB_Matrix_extractTuples -- the API requires the use to pass the size of the arrays that have been preallocated.

GrB_Matrix_export does not require the equivalent n_indptr, n_indices, and n_values.

Should we fix this inconsistency by adding these three arguments to GrB_Matrix_export and also introduce the GrB_INSUFFICIENT_SPACE error?

DrTimothyAldenDavis commented 2 years ago

Yes, absolutely. I brought this issue up in an email a while back (before I started using the github issue tracker), and I must have neglected to post this as a github issue. Sorry for the oversight.

mcmillan03 commented 2 years ago

The other difference is that exportSize reports size of each array in bytes and it looks like export expects size in bytes and not elements. I don't see a way around this for the values array...

DrTimothyAldenDavis commented 2 years ago

My GxBexport and GxBunpack methods use bytes, but they also rely on a move constructor so I don't have an exportSize.

I think bytes are easier and safer, and I would suggest we use bytes throughout, for all arrays for import/export. But # of entries would work too. It would depend on the type size, which ... well, there is no GxB_Type_size in the API. There's no way to ask a GrB_Type what its size is in bytes.

That makes the GrB_Matrix_export of a user-defined difficult. How many bytes should the values array be? It is nvals times the size of the type but what is the size of the type? The user application has to remember, and better get it right or else it's a segfault.

I really think GraphBLAS should have introspection methods that can query the type of a matrix, the size of a type, etc, but that's another set of issues.

DrTimothyAldenDavis commented 2 years ago

Wait ... my reading of the spec is that GrB_Matrix_exportSize reports the size of the array in # of entries, not # of bytes. So its consistent. The user must just be careful. To export a matrix of any type (built-in or user-defined) the user must know the exact type of the matrix (or at least the size of the type in bytes) and it can't guess wrong. There's no way to check. Guessing wrong is a segfault.

mcmillan03 commented 2 years ago

Actually the current documentation for export seems to imply that n_indptr and n_indices are number of elements (not bytes). E.g.

indptr: Must be at least as large as the corresponding value returned from \arg{GrB_Matrix_exportSize} times \arg{sizeof(GrB_Index)}"

Here is the docs from exportSize at issue:

\item[{\sf GrB_SUCCESS}] In blocking mode, the operation completed successfully. In non-blocking mode, this indicates that the API checks for the input arguments passed successfully. The sizes in bytes necessary for the export buffers will be written to {\sf n_indptr}, {\sf n_indices}, and {\sf n_values}.

DrTimothyAldenDavis commented 2 years ago

Oh. That is not consistent. I saw the former but not the latter.

DrTimothyAldenDavis commented 2 years ago

There is another problem, regarding the type of the matrix. The GrB_Matrix_export description states: "The type must match the type of A" for indptr, but the type is GrB_Index. Also for indices.

For the values array, the description states "Type must match the type of d" but there is no d. There is no way to check the type of the values array. There are no non-polymorphic variants. In my implementation, values is a void *. I have no way to know the type of the values array. I cannot return "GrB_DOMAIN_MISMATCH" since I cannot tell if the domain of A matches the type of the values array, since I do not have the type of the values array.

DrTimothyAldenDavis commented 2 years ago

Line 2407 for GrB_Matrix_import says regarding indptr: "Type must match the type of d.". That is not correct.

DrTimothyAldenDavis commented 2 years ago

Line 2395, the signature for GrB_Matrix_import, says that the values array has type "const values", but the function is not polymorphic. I wrote it as "const void " and I typecast (pun) internally to the type given by d.

mcmillan03 commented 2 years ago

After some thought (and discussion) this is what I am going to do:

all n_* variables contain number of elements
import and export functions are polymorphic on the values type (missing from chapter 5)
type of values array and scalar type in the matrix must be identical ("domains must match"...current language) which implies that casting will not be supported in the 2.0 release. We can safely relax to allow compatible domains later.
and am adding IN/OUT n_* parameters to export (just like extractTuples).

DrTimothyAldenDavis commented 2 years ago

Sounds good. I've done the first 3 bullet points now in my current draft (v6.0.0.alpha21). I haven't added the IN/OUT n* parameters yet. I assume those would go in the same places in the argument list as the input n parameters of GrB_Matriximport? That is, after the values array and before the GrB_Format parameter?

mcmillan03 commented 2 years ago

        GrB_Info GrB_Matrix_export(GrB_Index             *indptr,
                                   GrB_Index             *indices,
                                   <type>                *values,
                                   GrB_Index             *n_indptr, 
                                   GrB_Index             *n_indices, 
                                   GrB_Index             *n_values, 
                                   GrB_Format             format,
                                   GrB_Matrix             A);

DrTimothyAldenDavis commented 2 years ago

I've implemented the new non-polymorphic variants, and the two polymorphic functions GrB_Matrix_import and GrB_Matrix_export, with this signature. They're tested and working fine. The update will appear in v5.2.0.alpha21 and v6.0.0.alpha21 shortly.

mcmillan03 commented 2 years ago

Closing

GraphBLAS / graphblas-api-c

Array sizes in GrB_Matrix_export #60