8000 ndarray deserialization for any DataOwned by kylecarow · Pull Request #2 · RReverser/serde-ndim · GitHub
[go: up one dir, main page]
Skip to content

ndarray deserialization for any DataOwned #2

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.

Sign up for GitHub

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

Jump to bottom
Merged
RReverser merged 2 commits into from
Jun 7, 2025
Merged

ndarray deserialization for any DataOwned #2

RReverser merged 2 commits into from
Jun 7, 2025

Conversation

@kylecarow
Copy link
Contributor
@kylecarow kylecarow commented May 5, 2025 •
edited
Loading

This allows deserialization on more types, using the same bound as in ndarray:

https://github.com/rust-ndarray/ndarray/blob/8bd70b0c51e6ff3a6b6c2df94b4bcf9547dc9128/src/array_serde.rs#L144-L150

@kylecarow
Copy link
Contributor Author
kylecarow commented May 5, 2025

Also, is this bound on the NDim impl necessary?

serde-ndim/src/ndarray.rs

Lines 38 to 39 in a13bfac

where
S::Elem: Copy,

It forces me to add bounds on my types downstream. Removing it does not make any tests fail in this crate or in mine.

@RReverser
Copy link
Owner
RReverser commented May 5, 2025

Thanks for the PR!

Also, is this bound on the NDim impl necessary?

I can only assume I added it because it used to be required, but if tests pass without it as well, I'm happy for it to be removed as well.

@kylecarow
Copy link
Contributor Author
kylecarow commented May 5, 2025

I've added that change to this branch. Running cargo +nightly test --all-features locally doesn't find anything wrong with it, and my downstream code no longer needs to propagate Copy bounds.

Thanks for the quick review!

@kylecarow kylecarow mentioned this pull request May 7, 2025
Draft
@kylecarow
Copy link
Contributor Author
kylecarow commented Jun 6, 2025

Hey there! Any chance this PR could get a look over? Thanks!

@RReverser
Copy link
Owner
RReverser commented Jun 6, 2025

Sorry, fell off my radar. I just added CI rule, let's see if it passes and I'll merge.

@RReverser RReverser merged commit b9847f4 into RReverser:main Jun 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kylecarow @RReverser
0