[jira] [Commented] (MATH-915) Backward compatibility broken in "EmpiricalDistribution"

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-915) Backward compatibility broken in "EmpiricalDistribution"

ASF GitHub Bot (Jira)

    [ 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