commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gilles (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MATH-915) Backward compatibility broken in "EmpiricalDistribution"
Date Fri, 14 Dec 2012 14:26:13 GMT

    [ https://issues.apache.org/jira/browse/MATH-915?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13532325#comment-13532325
] 

Gilles commented on MATH-915:
-----------------------------

bq. The constructor that takes a RandomGenerator should not be deprecated

It is not deprecated.

bq. but it should call the superclass constructor

>From what I see, in "EmpiricalDistribution", the source of randomness is "RandomDataGenerator".
I indeed thought that it was redundant and first tried removing all reference to "RandomDataGenerator"
in "EmpiricalDistribution". But then it did not work with "ValueServer" since that class internally
uses an instance of "EmpiricalDistribution" created with a "RandomDataGenerator" argument!

Thus although I would gladly remove all usage of "RandomDataGenerator", it is not obvious
to do it in a clean yet backward-compatible way...
In the meantime, calling the superclass's constructor is not done on purpose: to avoid that
some methods use the superclass's (protected) "random" field while others use this class's
"randomData" (or the superclass's deprecated "randomData").

bq. What we might consider deprecating are constructors that take RandomDataImpl

This is exactly what I've done in the patch.

bq. any comments on how to better accomplish the simple goal of renaming RandomDataImpl would
be appreciated.

IMO, the problem is not with the renaming, it is having protected fields (instances of "RandomDataImpl",
"RandomDataGenerator", and "RandomGenerator") which prevent an easy transition path.
For one thing, in 4.0, the "random" field in "AbstractRealDistribution" should become private.

Until we can start making incompatible changes, I think that the current patch is probably
as good as any other half-baked solution: "Old" usage of "RandomDataImpl" will be delegated
to the new "RandomDataGenerator".
As I said above further cleanup should remove usage of _both_ classes in CM. IMHO, "RandomDataGenerator"
should only be a _user_ tool (syntactic sugar to bypass direct instantiation of a "distribution"
object).




                
> Backward compatibility broken in "EmpiricalDistribution"
> --------------------------------------------------------
>
>                 Key: MATH-915
>                 URL: https://issues.apache.org/jira/browse/MATH-915
>             Project: Commons Math
>          Issue Type: Bug
>            Reporter: Gilles
>            Priority: Blocker
>             Fix For: 3.1
>
>         Attachments: MATH-915.diff
>
>
> There is a binary-compatibility problem in {{o.a.c.m.random.EmpiricalDistribution}} (cf.
"Clirr" report).
> Usage of "RandomDataImpl" has been replaced by "RandomDataGenerator".
> However, unless I'm mistaken, none of those is actually necessary. Moreover, the "randomData"
field in this class "shadows" the (deprecated) protected field in the super class. Also, it
duplicates functionality (RNG) already present in the super class (through the the "random"
protected field).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message