search

janestreet / bin_prot

Binary protocol generator
MIT License
73 stars 21 forks source link

Add support for 32- and 64-bit floating point rank 3 arrays ("cubes") #21

Closed berke closed 4 years ago

berke commented 6 years ago

Hi,

Here's a signed commit. I got the OK from GHGSat, the terms at developercertificate.org are agreeable.

ghost commented 6 years ago

Sorry for the delay, I will have another look at this PR shortly. BTW, did you look at what I suggested in this comment? (I haven't looked at the diff of this PR yet).

aalekseyev commented 4 years ago

@berke, are you still interested in getting this merged? If you do, feel free to reopen it, but for now I'm closing it with the following justification.

I'm not entirely sure the support for cubes is worth having in bin_prot library. It's a very niche type, which I've never seen being used. Having extra code to support it means slightly longer compilation time and slightly larger binaries for everybody and more code to maintain for us.

In fact I'm already skeptical that the support for matrices is worth having in the first place. (we recently found a bug in sexp conversion of matrices that meant that literally nobody was using that)

To be clear, it makes sense to support the types even if they are used rarely, but maybe it makes sense to move such support to a separate library in that case.

berke commented 4 years ago

Hi Arseniy,

It's probably OK, but I've notified my ex-colleagues in case they are still using this.

What was the matrix bug? I think we were using (small) Lacaml mats in structure fields occasionally. If there were I/O bugs, they might have been attributed to something else.

aalekseyev commented 4 years ago

The bug was that sexp_of_float_mat in https://github.com/janestreet/sexplib/blob/master/src/conv.ml would write the data transposed (column-major order instead of row-major). So that it wasn't round-tripping with *_of_sexp.

berke commented 4 years ago

Thanks, good to know. IIRC we were storing and loading small covariance matrices from .sexp files, but they are symmetrical so they would not have been affected by the bug.