Bug: Real::signum() docs reference NaN

[ckaran]

At the top of the docs, it explicitly says that the Real trait is for things that might not have the concept of NaN. However, the docs for sqrt, abs, and signum each state that they are supposed to return Float::NaN under certain circumstances. So, should Real have a NaN representation or not?

[cuviper]

It should probably be stated as something unspecified / implementation-defined. Some Real types do have NaN, but you're right that it's not a requirement.

[ckaran]

It would probably be too big a change to do so, but would returning Result<Self,Error> make more sense? Maybe even Result<Self,BoxdynError> so that custom error types could be used.

[cuviper]

That would certainly be a breaking change.

[ckaran]

Is it worth doing it though? It feels cleaner and clearer than having to come up with a magic value that carries through (it's also more likely to catch mistakes than the equivalent of a silent NaN, and is more recoverable than panicking when you encounter a bad value).

[cuviper]

It might be worth considering in a semver-breaking update, but I'm not planning that any time soon. (I'm even considering going to 1.0 to reflect the stable status quo, with compatible re-exports in the 0.2 and 0.1 series.)

Do you have a particular type in mind? For the most part, num-traits is used as a generic interface for types that already implement such methods inherently, so I wonder what you would be doing for these methods on your own.

[ckaran]

I have my own custom type based on rug::Rational that implements an approximate square root. Since I don't have complex numbers implemented, returning the complex equivalent for negative values is meaningless. I don't currently have any need for the others, but that doesn't mean I won't in the future.

(I'm even considering going to 1.0 to reflect the stable status quo, with compatible re-exports in the 0.2 and 0.1 series.)

I thought you might be getting to that point, which is why I wanted to mention this to you now, before you go to 1.0. At least you'd be in your rights to make a breaking change while you're below 1.0...

[cuviper]

Yeah, technically breaking changes are fine going to 1.0, but in practice I really don't want to split the ecosystem.

[ckaran]

I understand. And I can see why you don't want to make a 0.3 series, but I'm also concerned about the rust ecosystem growing warts, and then being stuck with them. I see things that I consider to be warts in the standard library, but since they've stabilized there's no way of fixing them. I just want to make sure that we don't rush to stabilize, and end up with warts...

[cuviper]

The reality is that I already treat this like a stable 1.0 crate, but I know some folks would like to see that formally.

If we someday make breaking changes, that can just as well be 2.0.

[ckaran]

Got it. Well, maybe in the future then.

In the meantime, would you be willing to change the docs to make it clearer as to what should happen? That is, for those types that (for one reason or another) can't return a 'real' value, then they need to return some kind of signaling value? Or maybe panic instead?

[cuviper]

Now that I look closer, sqrt does say this:

If the implementing type doesn't support NaN, this method should panic if self < 0.

Quite a few others say something similar, like logarithms for self <= 0.

Would you like to add similar text where it's lacking? (e.g. signum)

I feel like abs should just remove the NaN statement though, as pretty much every method passes through NaN, and if the type never supports NaN then it's no bother.

[cuviper]

Actually, I guess signum is just a pass-through NaN as well. If we're going to say anything there, it could be something like "if the type supports NaN values, then signum returns NaN for those values."

[ckaran]

Just so I know, are you trying to mimic the feel of C/C++? That is, silent NaNs and all? If so, then this makes sense.

However, it feels like it goes against the rust philosophy of being a safe language. I feel like if a type cannot express the concept of NaN, then it should always panic. The problem with panics is that they make recovery more difficult, and in some cases trying to figure out the cause of a panic can be quite difficult (I have something that is the moral equivalent of a DAG of futures, so the backtrace looks like it came out of the executor, not the original source).

So I guess it comes down to what kind of 'feel' you want the API to have. A (possible) alternative is that you mention in the docs that all implementations of Real need to also have some reserved constants, one for each condition that can be returned (NaN, etc.). That will keep the API the same, and will allow end users to select which constants they provide.

Another alternative is to create a new optional trait depending on Error like so:

pub trait NumTraitError: Error {
    fn is_ok(&self) -> bool;
    fn is_err(&self) -> bool;
}

which types can implement in addition to Real. This will keep the API the same, only requiring some more documentation. Anyone that uses the API can then use trait bounds to verify that a given type implements NumTraitError if they need it, and everyone else keeps going as if nothing changed. It will cause a lot of people to scratch their heads and make comments about 'just return a Result', but if you don't want to change the API, this might be the path of least resistance.

[cuviper]

Just so I know, are you trying to mimic the feel of C/C++? That is, silent NaNs and all? If so, then this makes sense.

There are kind of two edges here. On one hand, num-traits is merely describing what the language and standard library are already doing, in this case providing a generic interface for existing f32 and f64 behavior. That does mean "silent NaNs and all," but that wasn't a decision on my part.

On the other hand, what we say in the docs may be seen somewhat prescriptive to downstream implementors, but I don't actually want to be too strict about telling them what to do. The panic recommendation seems to me as more of a caution for the end user of the trait, that panics are possible for types that can't do anything else. i.e. user beware, avoid invalid inputs if you can.

However, it feels like it goes against the rust philosophy of being a safe language. I feel like if a type cannot express the concept of NaN, then it should always panic.

Isn't that what the docs are saying right now? Either return NaN or panic? At least it does for sqrt, ln, etc. and we can improve stuff like signum.

A (possible) alternative is that you mention in the docs that all implementations of Real need to also have some reserved constants, one for each condition that can be returned (NaN, etc.).

If you carve out a NaN value for a type, you might as well implement Float, no? Well maybe you'd still have gaps like integer_decode... But if a type chooses to implement Real with a pseudo-NaN value as needed, that seems fine. For your rational, maybe that could be 0/0 if rug::Rational will allow that to exist.

I'm not sure it's possible to really have a generic trait that perfectly represents all possible "real" types, as they're all going to have subtle nuances. More practically, we just need T: Real to be a constraint that enables useful generic code.

---

Taking a step back, Real requires Copy, so doesn't that rule out your rug-based type entirely?

Also, num-traits isn't necessarily the be-all and end-all of numeric traits. For example, there is nalgebra::RealField with a smaller set of methods, even while they do use num_traits::Bounded and Signed.

[ckaran]

There are kind of two edges here. On one hand, num-traits is merely describing what the language and standard library are already doing, in this case providing a generic interface for existing f32 and f64 behavior. That does mean "silent NaNs and all," but that wasn't a decision on my part.

I understand, I guess I just started venting some of my frustrations. I'm sorry about that.

On the other hand, what we say in the docs may be seen somewhat prescriptive to downstream implementors,

Yup, that's exactly how I was thinking about it... :grin:

but I don't actually want to be too strict about telling them what to do. The panic recommendation seems to me as more of a caution for the end user of the trait, that panics are possible for types that can't do anything else. i.e. user beware, avoid invalid inputs if you can.

It's the end user of the types that are implementing the traits that I'm really concerned with. I view num-traits as a 'best practices' kind of crate, specifically so that silent NaNs aren't a thing. Not to say that I don't see your points about duplicating what the floating point types do, just observing that it would be nice if implementors understood their types better.

If you carve out a NaN value for a type, you might as well implement Float, no?

That's true!

Taking a step back, Real requires Copy, so doesn't that rule out your rug-based type entirely?

Yup, you're right, it does!

Also, num-traits isn't necessarily the be-all and end-all of numeric traits. For example, there is nalgebra::RealField with a smaller set of methods, even while they do use num_traits::Bounded and Signed.

Yup, you're right, there are a number of other crates out there (my cursory glance through them suggests Alga is a particularly good one). However, I still want all such crates (including num-traits!) to be as good as possible.

Regardless, I think that this issue is as solved as it's going to get. All that needs to be done is a little bit of documentation clean up, noting the issues, and letting implementors decide how they wish to proceed.