ndarray-vision icon indicating copy to clipboard operation
ndarray-vision copied to clipboard

Added serde support for `Image` type

Open volks73 opened this issue 2 years ago • 3 comments

Resolves #54.

A feature flag still needs to be added, but I wanted to share what I have so far to make sure I am on the right track.

I am not sure if this is a good implementation because I have added (de)serialization of the colour model as a string but during deserialization is actually never used. The colour model must be known ahead of time and used for the type annotation. The type is not determined from the "model" field. See the unit test for some context.

    #[test]
    fn deserialize_image_base() {
        const EXPECTED: &str = r#"{"data":{"v":1,"dim":[2,3,3],"data":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]},"model":"RGB"}"#;
        let actual: Image<u8, RGB> = serde_json::from_str(EXPECTED).expect("Deserialized image");
        assert_eq!(actual.model, PhantomData);
    }

If the model is not RGB then an error occurs instead of creating an image based on the colour model that is specified, but maybe this is correct way to do this? This is my first time really diving into manually implementing (de)serialization. Is there a way to deserialize into a "generic" Image.

volks73 avatar Jan 07 '23 21:01 volks73

So I would probably not serialize the colour model and instead rely on the user deserializing correctly - it's a bit of a pain, but given the user can implement the colour model trait and then give it a name which clashes with the ones implemented inside ndarray vision you can't really restore the colour model from the deserialized form (at least I don't see an ergonomic way to do so). So telling the user it should match and they have to look after it themselves is probably the least bad approach (given my initial thoughts on it).

I do have a feeling this is an example of why encoding/decoding to a standardised image format like png etc is potentially a better solution for most people and adding more encoder/decoders to the crate is probably worthwhile

xd009642 avatar Jan 12 '23 10:01 xd009642

Leaving out the color model from the (de)serialization would the serde support just a "pass through" for the serde support for ndarray. I agree this is probably the least bad approach.

volks73 avatar Feb 05 '23 16:02 volks73

Yeah, if you remove all the colour model based stuff you've added for ser/deser I'll give it another review pass and whenever it's merged cut a new release.

Also if you make serde optional (and only enable ndarray serde feature when it's enabled as well).

xd009642 avatar Feb 12 '23 17:02 xd009642