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

Implementations for u8->u64 and i8->i64 #873

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

timwedde
Copy link
Contributor

This is my first Rust-related PR, so apologies for any inadvertent mistakes.

I picked up dfdx yesterday but in my application I'm dealing with u8's and u16's a lot, and I figured it would be quite handy if we could properly interface them with Numpy instead of having to cast to f32/64 for the data exchange.

This PR adds support for the following dtypes:

  • u8
  • u16
  • u32
  • u64
  • i8
  • i16
  • i32
  • i64

I wasn't sure what the policy of this project is in regards to testing, so I added two basic save/load tests for u8 and i8 to prove the concept. Let me know if I need to add all of the other tests as well. That'd be quite a few permutations in this case, so I didn't think it worth it for the initial PR.

@ccaven
Copy link
Contributor

ccaven commented Oct 19, 2023

For future reference, make sure to run cargo fmt so the automated testing doesn't give you a "Some checks were not successful" :)

@timwedde
Copy link
Contributor Author

Good tip, thank you, though in this case the failing check actually stems from clippy in a file I didn't even modify: src/nn/add_into.rs:5:1. Not sure why that is, but I guess it might be carry-over from some other commits on master before I forked and made this PR.
cargo fmt does nothing in my case, so I assume everything is in order, at least formatting-wise.

@coreylowman
Copy link
Owner

@timwedde looks good to me, can you resolve conflicts and then I can force merge if pipelines don't pass

@timwedde
Copy link
Contributor Author

timwedde commented Dec 4, 2023

Alright, rebased onto main and checks all seem good.

@coreylowman coreylowman merged commit 4615ac1 into coreylowman:main Dec 4, 2023
4 checks passed
@coreylowman
Copy link
Owner

Thanks! 🎉

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.

3 participants