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

Feature f32 #8

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Feature f32 #8

merged 4 commits into from
Oct 6, 2023

Conversation

hakolao
Copy link
Contributor

@hakolao hakolao commented Oct 6, 2023

Hi, thanks for this library.

I needed f32 version, so I thought I'd PR it also.

Let me know if this won't do :D

@hakolao
Copy link
Contributor Author

hakolao commented Oct 6, 2023

I am using the contour_lines function only, and with the following use case: Turning 2d bitmaps into contour lines.

image

@mthh
Copy link
Owner

mthh commented Oct 6, 2023

Hi and thank you for your contribution (as well as for showing your example use case!).

I'll take a closer look and let you know if needed, or merge it and make a new release otherwise.

@mthh
Copy link
Owner

mthh commented Oct 6, 2023

I took a closer look at your code and I was wondering : do you have any particular reasons for using the "Float" crate?
I suspect it's not widely used (judging by its download statistics at https://crates.io/crates/Float).

I feel that by using the "num-traits" crate we could also make the code generic on f32 and f64, without this functionality being under a specific feature flag.

In most of the code, I guess it's a matter of changing the function signatures (fn within<T: num_traits::Float>(p: T, q: T, r: T) ...) and the Structs implementations (impl<T: num_traits::Float> ...), but in some (many ?) places we will probably also have to specify that T implements the CoordNum trait (https://docs.rs/geo/latest/geo/trait.CoordNum.html) since we're using geo_type::Coord (https://docs.rs/geo/latest/geo/geometry/struct.Coord.html) as the underlying type to describe points.

In fact, the "geo_types" crate (and therefore also "geo") uses "num-traits" to manage genericity over f32/f64 types.

What do you think about it ?

Anyway, I also think it would also be a good thing to be able to use this crate indifferently with f32 or f64 !

@hakolao
Copy link
Contributor Author

hakolao commented Oct 6, 2023

I didn't use any crate, just:

#[cfg(feature = "f32")]
pub type Float = f32;
#[cfg(not(feature = "f32"))]
pub type Float = f64;

@hakolao
Copy link
Contributor Author

hakolao commented Oct 6, 2023

Perhaps the more generic T route would be more idiomatic, I kinda went as simple as possible :), feature-gated it and made it so it doesn't break anything.

So yes, num-traits might be better. But do let me know if you prefer that rather than this type aliasing.

@mthh
Copy link
Owner

mthh commented Oct 6, 2023

I didn't use any crate

Indeed, sorry for the confusion !

Using num-traits requires quite some work (whereas your proposal is simpler and covers the use case of using f32 or f64).

I'll merge your PR and make a new release. Maybe in a future release I'll integrate num-traits.

@mthh mthh merged commit eec087b into mthh:master Oct 6, 2023
4 checks passed
@hakolao
Copy link
Contributor Author

hakolao commented Oct 6, 2023

great! Thanks :)

@mthh
Copy link
Owner

mthh commented Oct 6, 2023

Published on crates.io.
Thanks for your contribution :)

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