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

Re-enable num-traits under an optional feature #7

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

Conversation

althonos
Copy link

Hi Andrew!

First of all thanks a lot for developing this crate, I've been able to move some more of our data analysis pipeline to Rust thanks to it.

While I can understand why you want to get rid of the num-traits dependency in your context (at least, from what I got from the commit message of f19d626), it's actually super practical to have it available because you can depend on custom float types, the most interesting one for me being f16 from the half crate to work with a reduced memory footprint. The drop of num-traits support prevented me from updating to v0.3.

This PR keeps the best of both worlds by hiding the num-traits dependency behind a feature gate, which is disabled by default. Users like me who want to use the Float trait from num-traits can enable the feature, but by default kodama remains without dependency.

@BurntSushi
Copy link
Contributor

Hmmm, I'm not so sure about this. What if Kodama's Float trait was instead unsealed? Then presumably you could implement it yourself?

@althonos
Copy link
Author

althonos commented Mar 15, 2023

I don't think I could, because f16 is also an external type, so I couldn't implement the Float trait from kodama. And given that f16 already implements Float from num-traits, that would actually require double the effort for something that is there already...

@BurntSushi
Copy link
Contributor

Yeah the problem is that I'm just not sure I want to take on the extra burden of supporting num-traits optionally. I think the best I can offer is unsealing the Float trait. Then you can write a wrapper type that impls Float in this crate with anything that impls the corresponding trait in num-traits.

@althonos
Copy link
Author

Ah, that's unfortunate. I thought this would be okay considering the trait was already implemented in the previous versions...

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.

2 participants