sarah-ek / faer-rs

Linear algebra foundation for the Rust programming language
https://faer-rs.github.io
MIT License
1.75k stars 56 forks source link

add serde feature #98

Closed cramt closed 5 months ago

cramt commented 5 months ago

Added a feature to support serde serialization on the Mat type.

This is implemented by having a simple struct we can derive(Serialize, Deserialize) on, which represents the matrix in its serialized form, which is as a long vector along with the col length.

I wasn't sure if i should implement other matrix types than Matrix<DenseOwn> (which Mat ofc is an alias for), but im hoping i will get some feedback on that particular question

sarah-ek commented 5 months ago

looks great! i just have a couple remarks. both the number of rows and the number of columns should be (de)serialized, so that we can handle matrices of size 0. the other thing is i would prefer to avoid the vector allocation, and do the serialization on the data in-memory. but that can come later i suppose

codecov[bot] commented 5 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (ff226aa) 85.07% compared to head (725ec90) 85.09%. Report is 1 commits behind head on main.

Files Patch % Lines
faer-libs/faer-core/src/serde_impl.rs 97.22% 10 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #98 +/- ## ========================================== + Coverage 85.07% 85.09% +0.01% ========================================== Files 67 68 +1 Lines 86844 87236 +392 ========================================== + Hits 73884 74233 +349 - Misses 12960 13003 +43 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cramt commented 5 months ago

so when it comes to deserialization, we arent guaranteed to get the fields in any particular order, meaning that we could get data, then ncols, then nrows, in which case we would have to do the vec allocation, however im think i will just make it conditional and if we have the ncols and nrows when we deserialize the data then we make the matrix right here right now, otherwise allocate a vec we'll turn into the matrix once we have all the fields

sarah-ek commented 5 months ago

sounds good to me!

cramt commented 5 months ago

ehm, should we error in the case where someone has more elements serialized than nrows * ncols?, obviously we should in the inverse case where someone has less, but we could just ignore the extra if they have more

sarah-ek commented 5 months ago

i think returning an error would be a good idea. it sounds better to me than silently ignoring data that a user might have misplaced. and should help debug issues by reporting errors earlier rather than later

sarah-ek commented 5 months ago

looks good to me now. do you want to add something else or is this good to merge?

cramt commented 5 months ago

looks good to me now. do you want to add something else or is this good to merge?

i dont have anything else, so go right ahead and merge

sarah-ek commented 5 months ago

perfect! thanks a bunch for the contribution