gonum / matrix

Matrix packages for the Go language [DEPRECATED]
446 stars 51 forks source link

mat64: implement binary streamers #345

Closed sbinet closed 8 years ago

sbinet commented 8 years ago

Introduce:

for Dense and Vector. Re-wire MarshalBinary and UnmarshalBinary to use the above, with slight modifications.

Fixes #344.

sbinet commented 8 years ago

just a first stab.

also: should they return (int, error) or (int64, error) ?

io.Reader.Read([]byte) and io.Writer.Write([]byte) return (int, error)

but: io.ReaderFrom.ReadFrom(r io.Reader) and io.WriterTo.WriteTo(w io.Writer) return (int64, error)

jonlawlor commented 8 years ago

Then I would say (int64, error)

kortschak commented 8 years ago

I'm not entirely convinced that int64 is needed; io.ReaderFrom and io.WriterTo are generic pass through mechanisms that may take their bytes from arbitrarily long streams. We are marshaling and unmarshaling values that are in memory and so must be able to fit in a slice with len < maxInt for the architecture (and must already be smaller than maxInt for the architecture when considering \sum(sizeof(parts))). If the integer value is larger than maxInt (say unmarshaling a matrix on 386 that was marshaled on amd64) then the result is a non-nil error and the number of bytes that were actually read.

jonlawlor commented 8 years ago

I agree that it basically doesn't matter in practice. I'm just in favor of using the same convention as the standard lib whenever possible, and I think the io.Reader{From|To} cases are sufficiently close to the proposed MarshalBinary{From|To}.

kortschak commented 8 years ago

I don't think they are. The fundamental difference is that WriterTo and ReaderFrom deal with arbitrarily long streams; from the ReaderFrom docs, "ReadFrom reads data from r until EOF or error." UnmarshelFrom adds an additional constraint - or until enough data have been read to build the matrix. Semantically they are quite different despite the similarity of signature.

jonlawlor commented 8 years ago

Yes on second thought you are right.

sbinet commented 8 years ago

I'll wait for #354 to land before coming back to this, just to make sure I won't introduce performance regressions.

sbinet commented 8 years ago

PTAL.

went duplicating the streamers functions (it's not like the format will change anyways) added tests comparing streamers-based and non-streamers-based functions added tests for the error returned by the UnmarshalBinaryFrom

kortschak commented 8 years ago

The code looks OK, but we should have a test to guarantee congruity.

I'm also wondering if there should be a type mark at the beginning of the stored bytes.

sbinet commented 8 years ago

done. (all the n += nn)

sbinet commented 8 years ago

sigh... I need holidays.

thanks. done.

kortschak commented 8 years ago

LGTM