sparsemat / sprs

sparse linear algebra library for rust
Apache License 2.0
381 stars 45 forks source link

Serde tests failure #256

Closed vbarrielle closed 3 years ago

vbarrielle commented 3 years ago

I've recently noticed that the test tests/bincode_ser.rs was not being executed. I fixed that to make it run in the fix_serde_test_branch, but it reveals there's an error at serialization time for CsMatView: https://github.com/vbarrielle/sprs/runs/1581558055

I've tried looking into it, but so far I don't get what's wrong, the serialization code is the same than for CsVec, where it works... @mulimoen you know that serialization code much better than I do, do you have an idea?

vbarrielle commented 3 years ago

So the following diff fixes the issue:

diff --git a/src/sparse.rs b/src/sparse.rs
index c3fb133..0f86458 100644
--- a/src/sparse.rs
+++ b/src/sparse.rs
@@ -102,7 +102,7 @@ where
     storage: CompressedStorage,
     nrows: usize,
     ncols: usize,
-    #[cfg_attr(feature = "serde", serde(flatten))]
+    //#[cfg_attr(feature = "serde", serde(flatten))]
     indptr: IndPtrBase<Iptr, IptrStorage>,
     indices: IndStorage,
     data: DataStorage,

Looks like flattening the IndPtrBase is causing troubles, but I don't know why.

vbarrielle commented 3 years ago

I've found a similar issue https://github.com/servo/bincode/issues/245

vbarrielle commented 3 years ago

And according to https://github.com/servo/bincode/issues/167#issuecomment-653828999 this is not something we can be expecting a fix for.

Looks like we have two alternatives:

I'd really prefer the first solution.

vbarrielle commented 3 years ago

Something I don't get though: by removing the flatten attribute, serialization works, but I would expect the deserialization to fail since there should be one more level of structure. Or maybe that's not exposed with bincode.

vbarrielle commented 3 years ago

And indeed, if I test a roundtrip with serde_json without the flatten attribute, there is an error because what's serialized is no longer what was expected.

Looks like there's only Serialize that needs to be implemented manually.

mulimoen commented 3 years ago

I think we can cheat a little here and use the into container attribute. I'll test this approach

mulimoen commented 3 years ago

Serde is tricky, and I'm not very familiar with these sorts of problems. https://serde.rs/container-attrs.html suggests using into. I've tried this in https://github.com/mulimoen/sprs/tree/fix_serde_test, , but the requirement of Clone, gives the following error messages:

See errors ``` error[E0277]: the trait bound `IptrStorage: Clone` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ expected an implementor of trait `Clone` | = note: required because of the requirements on the impl of `Clone` for `sparse::CsMatBase` = note: required by `clone` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `IndStorage: Clone` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ expected an implementor of trait `Clone` | = note: required because of the requirements on the impl of `Clone` for `sparse::CsMatBase` = note: required by `clone` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `DataStorage: Clone` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ expected an implementor of trait `Clone` | = note: required because of the requirements on the impl of `Clone` for `sparse::CsMatBase` = note: required by `clone` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `N: Clone` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ expected an implementor of trait `Clone` | = note: required because of the requirements on the impl of `Clone` for `sparse::CsMatBase` = note: required by `clone` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `I: Serialize` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ the trait `Serialize` is not implemented for `I` | ::: /home/magnus/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.117/src/ser/mod.rs:247:18 | 247 | fn serialize(&self, serializer: S) -> Result | - required by this bound in `serialize` | = note: required because of the requirements on the impl of `Serialize` for `[I]` = note: required because of the requirements on the impl of `Serialize` for `&[I]` = note: required because of the requirements on the impl of `Serialize` for `CsMatBaseShadow` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) help: consider further restricting this bound | 87 | #[cfg_attr(feature = "serde", derive(Serialize + Serialize, Deserialize))] | ^^^^^^^^^^^ error[E0277]: the trait bound `N: Serialize` is not satisfied --> src/sparse.rs:87:38 | 87 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ^^^^^^^^^ the trait `Serialize` is not implemented for `N` | ::: /home/magnus/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.117/src/ser/mod.rs:247:18 | 247 | fn serialize(&self, serializer: S) -> Result | - required by this bound in `serialize` | = note: required because of the requirements on the impl of `Serialize` for `[N]` = note: required because of the requirements on the impl of `Serialize` for `&[N]` = note: required because of the requirements on the impl of `Serialize` for `CsMatBaseShadow` = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) help: consider further restricting type parameter `N` | 87 | #[cfg_attr(feature = "serde", derive(Serialize, N: Serialize, Deserialize))] | ^^^^^^^^^^^^^^ error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0277`. error: could not compile `sprs` To learn more, run the command again with --verbose. ```

A manual implementation might be the easiest next step