Carlo Hamalainen wrote: > So what do you think about this patch? > http://trac.sagemath.org/sage_trac/ticket/6827
Some random comments on http://trac.sagemath.org/sage_trac/attachment/ticket/6827/probability_distribution.patch Feel free to ignore this ranting. 757 def cum_distribution_function(self, x): If anything is spelled out, seems like it should be "cumulative". That said, in English "cdf" or "CDF" is a widely-understood abbreviation for cumulative distribution function. 758 """ 759 Evaluate the cumulative distribution function of 760 the probability distribution at ``x``. I guess x must be a number. If I were using this code I would really like for x to be a symbol too. 761 762 EXAMPLE:: 763 764 sage: T = RealDistribution('uniform', [0, 2]) Selecting the type by a string seems clumsy. Why not make Python work for you? e.g. T = distributions.uniform([0, 2]) . I guess you have RealDistribution here to indicate that the support of the density function is a subset of the reals. You can't exploit that programmatically: what subset is it, exactly? All of R, an interval, zero to infinity, what? Better to just indicate the support separately. e.g. uniform.support => interval or product of intervals, gaussian.support => all of R (however that's indicated), discrete.support => explicit enumeration (or implicit if you are clever), etc. (The reason I'm getting worked up over support is that it can be exploited to formulate an integral of foo(x)*pdf(x) where foo is a loss function and pdf(x) is a density. This is a computation of central importance in decision theory. You want people to use Sage for decision theory, right? There are various other integrals of interest, for which you'd need to know the support. Bayesian inference is all about the integrals. You want people to use Sage for Bayesian inference, right?) 765 sage: T.cum_distribution_function(1) 766 0.5 767 """ 768 if self.distribution_type == uniform: 769 return sage.rings.real_double.RDF(gsl_cdf_flat_P(x, self.parameters[0], self.parameters[1])) 770 elif self.distribution_type == gaussian: 771 return sage.rings.real_double.RDF(gsl_cdf_gaussian_P (x, self.parameters[0])) 772 elif ... This if--elif--elif-- ... seems clumsy. Once the type is selected, shouldn't you try to avoid branching to find the right function to call? Also, the purpose of each parameter is obscured here. If uniform, gaussian, etc were classes in a package of distributions, each one could have named parameters according to what's appropriate for them (a, b, mu, sigma, whatever). I've probably misunderstood what's going on here, if so, feel free to flame me as needed. FWIW Robert Dodier --~--~---------~--~----~------------~-------~--~----~ To post to this group, send an email to sage-devel@googlegroups.com To unsubscribe from this group, send an email to sage-devel-unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org -~----------~----~----~----~------~----~------~--~---
