Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added serde support for Image type #57

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

volks73
Copy link
Contributor

@volks73 volks73 commented Jan 7, 2023

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.

Christopher Field added 7 commits December 29, 2022 17:30
This is needed to make (de)serialization easier. I don't know of a
better way to handle this without defining the `Serialize` trait for
each `C` of the `ImageBase` or handling PhantomData.
@xd009642
Copy link
Member

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

@volks73
Copy link
Contributor Author

volks73 commented Feb 5, 2023

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.

@xd009642
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serde support
2 participants