Open YeungOnion opened 1 month ago
Also going to say that I'm not tied to this implementation, but I align well to the issue title. I don't think it makes sense to return Option
for all of them.
I think your proposal is good. It should be a clear benefit to avoid returning Option
fn mean
for a distribution which has no defined mean. Maybe a unit type ()
? Or should these be moved outside of trait Distribution
and only be implemented if a distribution supports them?rely on unimplemented!() over None when more correct
This is great, maybe remove the default implementations of mean
etc.[^1] while you're at it. It's not immediately clear if a missing implementation of mean
points to it being undefined or not implemented yet. That should probably be handled by the implementor.[^2]
T: Float
trait boundtrait Distribution
in module statrs::statistics
and not statrs::distribution
again?[^1]: Excluding std_dev
[^2]: Another thing: The default implementations have function documentation following the pattern "Returns X, if it exists". But we know that all of these return None
unless overridden by a manual implementation (which should have its own, overriding docs) so this text almost always actually means "Returns None"
I hadn't thought of this one, but the API I designed doesn't work well for fallible types,
I can't impl<T> Covariance<Option<T>> for Option<T>
separately from impl<T> Covariance<T> for T
because of the standard deviation[^impl_cov_for_option_t]. Standard deviation could be its own trait or simply implemented on types where it is useful[^1], and then I could drop the "note to implementors" on std_dev
method, quoted what I came up with below,
[^impl_cov_for_option_t]: I don't think it would be good semantics to do so, even if it is more ergonomic, calling covariance methods in map
on Option<T: Covariance>
types would be better than Option<f64>::default().forward(1)
.
[^1]: at the moment, this is my preference.
Note for implementors
Associated types should capture semantics of the distribution, e.g. [
Option
] should be used where moments is defined or undefined based on parameter values. [()
] is used for moments that are never defined
I can't
impl<T> Covariance<Option<T>> for Option<T>
separately fromimpl<T> Covariance<T> for T
because of the standard deviation.
I'm not sure I understand this point.. can you elaborate or maybe give an example? It seems possible but I'm not sure I understand your proposal correctly.
EDIT: I just saw #304. I'll take a look and see if I can't figure it out
To couple the types for the return of variance
and std_dev
I have
Type Var: Covariance
and the return type of std_dev is <Var as Covariance>::V
Perhaps I should be using the return of variance
as the type from dense, Covariance::M
, but the other option is not constraining a type relationship between variance and std_dev by removing std_dev or adding another associated type (which I think is unnecessarily flexible).
EDIT: on mobile, but within 24h I can push the WIP commit. It won't compile, but it's where I first noticed it; was while implementing moments for the F-distribution.
Many distributions will have some summary statistics regardless of their parameters so
Option<T>
should not be required, e.g. it is not semantically accurate to require unwrapping the mean of a binomial. Regardless of the sample probability, a validBinomial
type will always have some mean, but student's distribution require at least one dof, cauchy has too much tail and has no mean is never valid.proposed solution
In lieu of generics, we can use associated types for the
statistics::traits::Distribution
trait[^renameDistribution]. I don't believe this would bound us in terms of generic numerics in the future either, as long as the associated type could support the generic as well, i.e.MeanN
andVarianceN
intostatistics::traits::Distribution
and addCovariance
andEntropy
traits208 proposed a way to specify
Covariance
unimplemented!()
overNone
when more correct. Defaulting toNone
when we don't have a simple expression isn't correct (may also be a little more motivating for someone to implement it over panicking).Covariance
on matrices, Cholesky decompositions, vectors, and likely floats.!
before being plain ol'unimplemented!()
before being implemented.Sample code of the traits I propose adding.
[^renameDistribution]: I also think renaming it could be helpful, unsure what is better, but since most of them are moments, or central moments, we could make entropy it's own, as entropy is always scalar and have a Moments trait. I think much of this is motivated from reference implementations in Math.NET's interfaces since that's how this project started.