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

Add RealNum trait for real data types (Float, but without floating-point specific features) #23

Merged
merged 5 commits into from
Jan 19, 2018

Conversation

yoanlcq
Copy link
Contributor

@yoanlcq yoanlcq commented Jan 5, 2018

This is supposed to fix #19; I assumed going ahead would be better than bumping the thread.

In any case, I understand that it is a quite significant addition and won't mind too much if it doesn't make it.

This adds a new RealNum trait, along with a universal impl impl<T: Float> RealNum for T { ... }.
Therefore, this shouldn't be a breaking change, except in places where both traits are imported (which obviously only happened in a few places in this crate).

The intent is that generic code may prefer to use RealNum instead of Float when floating-point isn't a requirement. In the future (next major version ?), I guess Float could be made to only provide floating-point-specific features on top of RealNum.

Most of the code+doc was copy-pasted from Float, but the doc comments should be up-to-date with the situation; Float only makes an appearance when talking about NaN and infinity.

Issues I've seen :

  • RealNum might not be the name we want;
  • I've mentioned that sqrt() is allowed to panic if the input is negative and has no meaningful NaN representation;
  • Should we do that too for e.g log() ? Like sqrt(), it's supposed to return Nan when x < 0.

Thanks for your time. :)

@yoanlcq yoanlcq changed the title Add RealNum trait Add RealNum trait for real data types (Float, but without floating-point specific features) Jan 5, 2018
@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 5, 2018

Wait, I just realized this is a breaking change for those that do global imports like use num_traits::*.
Guess we should remove pub use realnum::RealNum;.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 5, 2018

(last commit was supposed to be named "Remove pub use realnum::RealNum::*;", but Bash tried to interpret the backticks, and git push was in the same line. Today I Learned.)

@fabianschuiki
Copy link
Contributor

This is a neat trait. I've been toying around with an implementation of fixed-point numbers where this trait would be quite useful. I was wondering if it would be useful to introduce additional subtraits, e.g. Rounding to capture floor(), ceil(), round(), trunc(), and fract(). A type might wish to implement rounding, take my fixed-point use case for example, but not be forced to also implement transcendentals like log or exp.

How about changing the trait name to Real, similar to Float, Signed, Unsigned, etc.?

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 6, 2018

I like Real, it's shorter than RealNum and no less clear!

I think subtraits as you mentioned would be quite elegant, but it's difficult to decide where to draw the line (i.e how far do we go ? and how often would generics need e.g Rounding instead of plain Real ?).
For instance, nobody said that all real-number types were Copy (what about bignum-style real numbers, backed by a Vec mantissa, for instance ?) but Float still enforces it, which has a huge impact on existing generic code; But somehow, it's still fine.
To me, the advantages of sub-traits would be 1. less pressure on implementors, and 2. less restrictive bounds in generic APIs (but, that could imply more verbose bounds).

Today a possible solution for 1. would be to use unimplemented!{} in appropriate items and just make sure they aren't used; Guess that's a reasonable thing to do in the meantime, especially in experimental projects.

I gave a try at splitting functionality into such subtraits below (pseudo-Rust), and now I find the idea to be a bit scary.

// Q: How to split all of this ? Do we want to ?
pub trait ????? {
    // MinMax
    fn min_value() -> Self;
    fn min_positive_value() -> Self;
    fn max_value() -> Self;

    // Sign-related
    fn abs(self) -> Self;
    fn abs_sub(self, other: Self) -> Self;
    fn signum(self) -> Self;
    fn is_sign_positive(self) -> bool;
    fn is_sign_negative(self) -> bool;

    // Simple arithmetic ops
    fn mul_add(self, a: Self, b: Self) -> Self;
    fn recip(self) -> Self;
    // These two are both simple arith. and constants
    fn to_degrees(self) -> Self { ... }
    fn to_radians(self) -> Self { ... }

    // Constants
    fn epsilon() -> Self { ... }

    // Comparison-related
    fn max(self, other: Self) -> Self;
    fn min(self, other: Self) -> Self;
}
pub trait Rounding {
    fn floor(self) -> Self;
    fn ceil(self) -> Self;
    fn round(self) -> Self;
    fn trunc(self) -> Self;
    fn fract(self) -> Self;
}

// That's still a lot to implement. Does it solve our problem ?
// Otherwise, if we split all of this, how many traits would we end up with,
// and what would their names be ?
pub trait Transcendental {
    fn powi(self, n: i32) -> Self;
    fn powf(self, n: Self) -> Self;
    fn sqrt(self) -> Self;
    fn exp(self) -> Self;
    fn exp2(self) -> Self;
    fn ln(self) -> Self;
    fn log(self, base: Self) -> Self;
    fn log2(self) -> Self;
    fn log10(self) -> Self;
    fn cbrt(self) -> Self;
    fn hypot(self, other: Self) -> Self;
    fn sin(self) -> Self;
    fn cos(self) -> Self;
    fn tan(self) -> Self;
    fn asin(self) -> Self;
    fn acos(self) -> Self;
    fn atan(self) -> Self;
    fn atan2(self, other: Self) -> Self;
    fn sin_cos(self) -> (Self, Self);
    fn exp_m1(self) -> Self;
    fn ln_1p(self) -> Self;
    fn sinh(self) -> Self;
    fn cosh(self) -> Self;
    fn tanh(self) -> Self;
    fn asinh(self) -> Self;
    fn acosh(self) -> Self;
    fn atanh(self) -> Self;
}
// Then in any case :
pub trait Real: ????? + Rounding + Transcendental + Num + etc... {}

Say my vector_magnitude function only needs sqrt(), does it mean we should make an Sqrt trait ? Even in this instance, I can't seem to choose between "yes" and "no" - there are too many matters of personal taste and convenience into the mix IMO.

I feel like Float and Real are those kind of places where it's best to have everything in a single place. Also, I believe that Float was designed around Rust's floating-point types and modules, the intent being "Okay, Float is any type that has the same API as f32 or f64".
At the same time, I dream of "single-function" traits such as Sqrt and Cos (or SinCos, whatever). But, because this hasn't been done from the start, it's probably not a good idea to suddenly move to doing this.

The num crates have historically been (rightfully) conservative, so I doubt its maintainers would like the idea; I'm already not sure about RealNum, so I'd rather wait for their opinion/decision. :)

EDIT: Let's also not forget about the possible impact on Integer, Signed and friends. If we start splitting Real into sub-traits, if would be weird if the others didn't follow as well. To summarize, this would have too much of an impact in the public API, probably even for next major increase.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 6, 2018

BTW, I wonder if, while we're at it, we should make Real NOT assume Copy. This would open the gate to (hypothetic) BigFixed or BigRational types, and shouldn't change the API (users can clone() as needed before calls).

However, such types would still prefer not to be consumed by calls, and all of num-trait's APIs already consume values instead of borrowing. So it's probably best to keep Copy.
(Well, I guess this was a random thought - it is way besides the point of this PR.)

@fabianschuiki
Copy link
Contributor

Now that I come to think of it, starting to split things into subtraits leads to a lot of trait proliferation, as you pointed out. Especially if we go with it ad nauseam, we end up with one trait per function. Eventually we'll construct umbrella traits like Rounding: Floor + Ceil + Round + Trunc + Fract, and umbrella types of umbrella types like Real: Rounding + Transcendental + .... Quite the trait zoo.

Is there a consensus as to where the line is drawn at the moment? I see that Integer, Signed, Unsigned do kind of a split into subtraits; then there is WrappingAdd, WrappingSub, etc., but only a Saturating trait. Was this done on purpose or is this coincidental/historical?

One effort I could imagine would be to extend the wrapping, saturating, and checked modules with traits such that they match what's available in stdlib. Also, since there's some precedence for a Saturating trait, maybe a Checked and Wrapping trait could be added. Another option would be to go the route stdlib is taking, and add generic Checked<T> and Saturating<T> structs.

@cuviper cuviper mentioned this pull request Jan 13, 2018
@cuviper
Copy link
Member

cuviper commented Jan 13, 2018

I'd prefer to avoid too many traits here. If you can think of a reasonable type that could only implement part of the functionality, then perhaps that does deserve a separate trait, but let's definitely not get down to one trait per method.

Is there a consensus as to where the line is drawn at the moment? I see that Integer, Signed, Unsigned do kind of a split into subtraits; then there is WrappingAdd, WrappingSub, etc., but only a Saturating trait. Was this done on purpose or is this coincidental/historical?

Historical, and such idiosyncrasies are part of why this stuff never got stabilized while it was still in the standard library. You could describe most of num as "good enough to be useful, but not ideal." :)

We definitely need to settle on these questions and things like Copy here before anything is merged. I think what you've got is OK, if you're happy with it. There's also considerations like #16 and #24 for no_std -- with all the transcendental methods, we'd probably have to exclude Real for std builds only. (which isn't the end of the world...)

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 13, 2018

I'm fine with the current situation in this PR.

This may be the single opportunity we have to split Real into several subtraits (as in the example mockup I wrote earlier), but I would still rather avoid this; reasons include :

  • It's inherently opinionated;
  • It doesn't feel consistent; Real is intended as a subset of Float;
  • It would make trait bounds in generics more verbose (unless we abuse "trait inheritance").
  • It's not that important to do : usually, it's easy to implement most of the items; the only problem appears to be transcendental functions, in which case the two options are ether to bite the bullet or insert unimplemented!{} // Pull Requests welcome! etc.

About Copy, my decision would be to leave Real be Copy, just like Float. Again, this might look like a missed opportunity, but it would have way too much of an impact in the API, and break too many assumptions that are somewhat set in stone today.
I think that removing Copy from Float, Real and others would only make sense as part of a huge (and painful) future overhaul of num.
In any case, I hardly picture somebody thinking "Oh God, I'm so needing this Vec4<BigFloat>". As you said, num has to be good enough, not perfect. If such functionality is critical for a handful of projects in the wild, people can implement or copy-paste what they need and be done with it. Isn't that what people would do in languages other than Rust anyway ? 😄

@fabianschuiki any thoughts before this possibly gets merged ?

About issues related to no_std, I think that whatever is done to Float should be done to Real as well. I think I saw solutions to the underlying problems, but I need to make sure they're sound, and if they are, this PR is not really the place to discuss them.

@fabianschuiki
Copy link
Contributor

"Good enough to be useful" seems like a good compromise. Especially because you can make the division into subtraits arbitrarily fine-grained, I agree that erring on the side of less traits is the better choice. You also make good points that if need be, you can always roll your own custom trait; and there's no shame in using unimplemented!().

I think this PR is good as it is!

@porky11
Copy link

porky11 commented Jan 18, 2018

If the goal is minimizing the number of traits I'd rather remove the Num and the NumOps trait.
The number types required by my types never require Rem.
And when I use the Float trait, i only use the mathematical functions, never float specific ones.

@cuviper
Copy link
Member

cuviper commented Jan 18, 2018

IMO it's fine that the trait may require more things than you need, as long as it's not too much burden for the types that might want to implement this trait. You can always write your own local trait if you want something with reduced constraints.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 18, 2018

@porky11 thanks for your input! Please correct me if you think I misunderstood some of your statements.

If the goal is minimizing the number of traits I'd rather remove the Num and the NumOps trait.

Actually the goal is really to provide a Float trait without the float-y bits, which is interesting for some kind of types. I think Num and NumOps make sense for them : In my lib, for instance, I'd rather replace Float by Real than by Real + Num + NumOps.

The number types required by my types never require Rem.

Conceptually this could be also true for a bunch of traits other than Rem, but a line has to be drawn at some point, and I think people would rather expect real number types to implement Rem.
If Rem doesn't make sense for your own real number types, I'd say just use unimplemented!{}.

And when I use the Float trait, i only use the mathematical functions, never float specific ones.

Me too! In these case we'll want to replace all Float bounds by Real, which is less restrictive.


Also I've read your issue and one part I find very interesting is how you concern yourself with not just real number types, but most mathematical types such as matrices and quaternions, in which case e.g Div and Rem don't make much sense.
I like very much the idea, and this is conceptually how it ought to be done, BUT some bits are trying to be too generic IMO (or at least, for this crate to provide).
I like the Power and Trigonometric split, but how many types would implement one and NOT the other ? etc. (I thought about matrices, but even so, it's weird).

Truth to be told, implementing "Float items" for matrices, quaternions, etc. is more the exception than the norm, and the problems you mentioned can be solved today with the current situation (it's not perfect, but can reasonably be dealt with).
You probably did this already, but looking at existing maths crates for inspiration is a good idea (I'm thinking about nalgebra and cgmath); they somehow managed to solve these problems in a way that looks (and feels) reasonably good.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 18, 2018

I just realized there's one unsolved question before we should merge (if we want to).
In a bunch of items in Real, some functions are expected to return NaN on error, but this value only exists for Floats.

Should we say that all methods are allowed to panic instead when the type has no meaningful NaN representation ?
I speculatively did this for sqrt(), but not for some other items.

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 18, 2018

Ok I think everything is fine now.

@porky11
Copy link

porky11 commented Jan 18, 2018

Just a real type also is a good enough solution for most types.
The problem is, that I'd like to be able to implement some of my types for integer-like types.
I want to implement geometric algebra, where I need my numbers to work like real numbers. According to your split, my numbers would need to implement Add, Sub, Mul and Div, and additionally Transcedental trait (i.e sqrt, because they work similar to geometric algebra).
I like this more than splitting further to Trigonometric and Power.
Having a single Sqrt trait seems stupid, since when you can implement one Transcedental function, you can implement all of them, I think. It may even be possible to add default implementations.

Maybe powi could get it's own trait, because it can be defined over every type T: Mul<Output=T>, by just iterating over the value. (Maybe it would have to implement One too)

My new type would also be able to implement Transcedental, if you would be able to define multiple kinds of output types. Because I want to be able to specialize some types, so that in my case, for example Vector::exp(self) will result in the type Multivector and Bivector::exp(self) will result in Rotor. So I probably first try to implement traits in my own crate, and if I think, it's useful for other use cases, too, I'll propose such a trait here.

@cuviper
Copy link
Member

cuviper commented Jan 18, 2018

@porky11

Maybe powi could get it's own trait, because it can be defined over every type T: Mul<Output=T>, by just iterating over the value. (Maybe it would have to implement One too)

FWIW, there's already a pow function specified this way. It can't handle negative exponents though, nor can it lower to the intrinsic powi for floats, but it does work well for handling positive exponents generically.

@yoanlcq final review note -- do we really want the default implementations for epsilon, to_degrees, and to_radians? Float only has these defaults for compatibility, since they were added after the fact and would have otherwise been breaking changes. (And see #4 proposing to clean up epsilon.)

@yoanlcq
Copy link
Contributor Author

yoanlcq commented Jan 18, 2018

do we really want the default implementations for epsilon, to_degrees, and to_radians?

Put this way, I've taken a good look and I'm pretty sure we don't. Gonna remove them now for the better.

@cuviper
Copy link
Member

cuviper commented Jan 19, 2018

OK, let's merge. Thanks everyone, I appreciate the collaboration on this!

bors r+

bors bot added a commit that referenced this pull request Jan 19, 2018
23: Add RealNum trait for real data types (Float, but without floating-point specific features) r=cuviper a=yoanlcq

This is supposed to fix [#19](#19); I assumed going ahead would be better than bumping the thread.  

In any case, I understand that it is a quite significant addition and won't mind too much if it doesn't make it.

This adds a new `RealNum` trait, along with a universal impl `impl<T: Float> RealNum for T { ... }`.  
Therefore, this shouldn't be a breaking change, except in places where both traits are imported (which obviously only happened in a few places in this crate).

The intent is that generic code may prefer to use `RealNum` instead of `Float` when floating-point isn't a requirement. In the future (next major version ?), I guess `Float` could be made to only provide floating-point-specific features on top of `RealNum`.

Most of the code+doc was copy-pasted from `Float`, but the doc comments should be up-to-date with the situation; `Float` only makes an appearance when talking about NaN and infinity.

Issues I've seen : 
- `RealNum` might not be the name we want;
- I've mentioned that `sqrt()` is allowed to panic if the input is negative and has no meaningful NaN representation;
- Should we do that too for e.g `log()` ? Like `sqrt()`, it's supposed to return Nan when `x < 0`.

Thanks for your time. :)
@bors
Copy link
Contributor

bors bot commented Jan 19, 2018

Build succeeded

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.

Extract some kind of RealNumber trait out of Float and make Float require it
4 participants