osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [All][RNG] Fix minor design issue (Was: commons-rng git commit: [...] deprecations. [...])


I think we can make your changes Gilles, if we do a 2.0. Would you prefer to go that route? If we go with 1.1 (other than via deprecation), I think we’re stuck behind a bunch of -1 votes, and Id rather not roll an RC to that audience. I can work on the BC changes or the version changes between now and next week. 

-Rob

> On Aug 9, 2018, at 10:49 AM, Gary Gregory <garydgregory@xxxxxxxxx> wrote:
> 
>> On Thu, Aug 9, 2018 at 8:47 AM Gary Gregory <garydgregory@xxxxxxxxx> wrote:
>> 
>> 
>> 
>>> On Thu, Aug 9, 2018 at 7:25 AM Rob Tompkins <chtompki@xxxxxxxxx> wrote:
>>> 
>>> 
>>> 
>>>> On Aug 9, 2018, at 9:17 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>>> wrote:
>>>> 
>>>> On Thu, 9 Aug 2018 08:43:34 -0400, Rob Tompkins wrote:
>>>>>> On Aug 9, 2018, at 8:38 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>>> wrote:
>>>>>> 
>>>>>>> On Thu, 9 Aug 2018 08:22:32 -0400, Rob Tompkins wrote:
>>>>>>> Are we instead trying to remove the “extends” from all of those
>>> classes?
>>>>>> 
>>>>>> I'm not sure what you mean, sorry.
>>>>>> 
>>>>>> "SamplerBase" was meant has a utility/shortcut/boiler-plate in order
>>>>>> to copy the "rng" argument in one place, instead of having a field
>>>>>> in each sampler class. It is purely internal to this library (in C++,
>>>>>> inheritance would have been "private”).
>>>>> 
>>>>> Are we trying to remove SamplerBase all together?
>>>> 
>>>> That would break BC the same way.
>>>> I wouldn't mind since the shortcut is not really significant.
>>>> 
>>>> But my reasoning would be the same: correct usage does not refer
>>>> to these methods so the fix could be done now, with minimal risk
>>>> (due to low usage of a new component).
>>>> 
>>>>>>> My thought was to override the method and javadoc locally (in the
>>>>>>> actual subclass) so that we can give reasonable deprecation messages,
>>>>>>> but that’s only a thought. I’m ok with whatever.
>>>> 
>>>> It's overkill in this context.
>>>> We shouldn't be rigid about the "no BC in minor release" rule;
>>> 
>>> I’m a tad confused here. I thought that the whole point of everything
>>> that we do in terms of the way we version and name packages
>>> was specifically driven at the fact that we’re completely rigid about “no
>>> BC breakage in minor releases.”
>>> 
>>> I’ve not been around the project long enough to know this with certainty,
>>> but that definitely is the vibe that I get. I would think that such RCs
>>> would generally get -1’s from people.
>>> 
>>> Generally I’m open to whatever. I’m just operating on what I perceive to
>>> be precedent.
>>> 
>>> @Gary - what’s the call here on the flexibility of BC incompatible
>>> changes in a minor release?
>>> 
>> 
>> BC is binary and we should not break stacks. You really don't want to
>> create jar hell for our users. Think about deep transitive dependencies. An
>> end use or developer is not going to care 2 cents about "correct vs
>> incorrect API usage" when a component depends on this depends on that and
>> so on, ends up breaking his or her whole build because a low level
>> component has decided that breaking BC was OK.
>> 
> 
> I'll also note that I am not close enough to this component to opine on the
> quality of the API or the workarounds discussed. My point is only "Don't
> create jar hell; don't break BC."
> 
> Gary
> 
> 
> 
>> 
>> Gary
>> 
>> 
>>>> enforcing it is worse for everybody:
>>>> * bona fide users will be forced to modify their source and
>>>>  recompile in order to benefit from the performance boost
>>>>  introduced in version 1.1, and
>>>> * bad usage (if it exists at all) could continue unsuspected.
>>>> 
>>>>>> The message would aim at the wrong target: for developers of this
>>>>>> library, there is no deprecation (the boiler-plate code is there
>>>>>> to be used); for application developers, the class should not be
>>>>>> there to be used (hence: package private).
>>>>> 
>>>>> Then, I guess, we should simply state that we want to eventually
>>>>> delete these methods and they should not be consumed. Yeah?
>>>> 
>>>> No (cf. above); I propose to delete them *now*.
>>>> 
>>>>> Like I
>>>>> said…I’m up for whatever. I’m just trying to balance yours and Gary’s
>>>>> points and find a good middle ground that everyone can live with.
>>>> 
>>>> Which are the two sides for which you try to find a middle ground?
>>>> At any rate it would not be "good" if it makes developers and users
>>>> unhappy for a case that probably doesn't exist out there.
>>>> Tools such as Clirr are a great help, but they shouldn't induce us
>>>> to take the wrong course only to have the pleasure to contemplate a
>>>> clean report. ;-)
>>>> Thanks to Clirr (and Gary), we've become aware of a design issue.
>>>> We can decide to resolve it as it was done in the past, with IMHO
>>>> zero inconvenience (except for the Clirr "unclean" report).
>>>> 
>>>> Regards,
>>>> Gilles
>>>> 
>>>>> -Rob
>>>>> 
>>>>>> 
>>>>>> Gilles
>>>>>> 
>>>>>>> -Rob
>>>>>>> 
>>>>>>>> On Aug 9, 2018, at 7:02 AM, Gilles <gilles@xxxxxxxxxxxxxxxxxxxxx>
>>> wrote:
>>>>>>>> 
>>>>>>>> Hi all.
>>>>>>>> 
>>>>>>>>> On Wed, 08 Aug 2018 19:59:24 +0200, Gilles wrote:
>>>>>>>>> Hello Rob.
>>>>>>>>> 
>>>>>>>>>> On Wed, 8 Aug 2018 12:44:16 -0400, Rob Tompkins wrote:
>>>>>>>>>> @Gilles - thoughts here?? Just kinda what I was thinking, but I’m
>>>>>>>>>> only a +0 on this change. So, if you want to revert it before
>>> going up
>>>>>>>>>> with 1.1, that’s fine.
>>>>>>>>> 
>>>>>>>>> I don't understand this change after what I answered to Gary's
>>> strange
>>>>>>>>> proposal.
>>>>>>>>> The comment is *wrong*.  As I said previously, the base class
>>> provides
>>>>>>>>> boiler-plate code; that is *not* deprecated.  The issue is that
>>> those
>>>>>>>>> methods were not meant to be used further down the hierarchy (in
>>> user's
>>>>>>>>> subclasses). [And now, furthermore, they are not used anymore in
>>> class
>>>>>>>>> "PoissonSampler", following the change in RNG-50 (delegation to
>>> other
>>>>>>>>> classes).]
>>>>>>>>> The sampler classes should have been made "final"; but now, this
>>> change
>>>>>>>>> would also be "breaking" (even though I doubt about legitimate
>>> use-cases
>>>>>>>>> for inherithing from the sampler implementations).
>>>>>>>> 
>>>>>>>> Assuming it's unlikely that application developers would have
>>>>>>>> * called protected methods of "SamplerBase",[1]
>>>>>>>> * create subclasses of "SamplerBase",[1]
>>>>>>>> * create subclasses of the sampler classes,[1]
>>>>>>>> I suggest to
>>>>>>>> * make "SamplerBase" package private
>>>>>>>> * make the sampler clases "final".
>>>>>>>> 
>>>>>>>> The above are the *less* intrusive fixes, as they would only
>>>>>>>> potentially impact application developers by making them aware
>>>>>>>> that they have made incorrect usage of an *inadvertently*
>>>>>>>> public API.  Correct usage will not be impacted even though
>>>>>>>> the change is not BC.[2]
>>>>>>>> 
>>>>>>>> Any objection to that fix?[3]
>>>>>>>> 
>>>>>>>> Regards,
>>>>>>>> Gilles
>>>>>>>> 
>>>>>>>> [1] Rationale: (a) There is no reason to do that and
>>>>>>>>            (b) "Commons RNG" isn't much used at this point:
>>>>>>>> 
>>> http://mvnrepository.com/artifact/org.apache.commons/commons-rng-sampling/usages
>>>>>>>> [2] I recall that a similar situation (BC breaking in a minor
>>>>>>>> release) occurred in CM, at a time where the number of
>>>>>>>> potential users was much larger.
>>>>>>>> [3] I'll add a prominent warning in the changelog to the effect
>>>>>>>> that people wanting to continue with incorrect usage of the
>>>>>>>> samplers should not upgrade. ;-)
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Please revert.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Gilles
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> -Rob
>>>>>>>>>> 
>>>>>>>>>>> On Aug 8, 2018, at 12:42 PM, chtompki@xxxxxxxxxx wrote:
>>>>>>>>>>> 
>>>>>>>>>>> Repository: commons-rng
>>>>>>>>>>> Updated Branches:
>>>>>>>>>>> refs/heads/1.1 50b984b1d -> f8159f28a
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Adding PoissonSampler deprecations. Use the correct faster
>>> public methods
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-rng/repo
>>>>>>>>>>> Commit:
>>> http://git-wip-us.apache.org/repos/asf/commons-rng/commit/f8159f28
>>>>>>>>>>> Tree:
>>> http://git-wip-us.apache.org/repos/asf/commons-rng/tree/f8159f28
>>>>>>>>>>> Diff:
>>> http://git-wip-us.apache.org/repos/asf/commons-rng/diff/f8159f28
>>>>>>>>>>> 
>>>>>>>>>>> Branch: refs/heads/1.1
>>>>>>>>>>> Commit: f8159f28a52197d0e7b55e39b115702147cf57a0
>>>>>>>>>>> Parents: 50b984b
>>>>>>>>>>> Author: Rob Tompkins <chtompki@xxxxxxxxx>
>>>>>>>>>>> Authored: Wed Aug 8 12:42:45 2018 -0400
>>>>>>>>>>> Committer: Rob Tompkins <chtompki@xxxxxxxxx>
>>>>>>>>>>> Committed: Wed Aug 8 12:42:45 2018 -0400
>>>>>>>>>>> 
>>>>>>>>>>> 
>>> ----------------------------------------------------------------------
>>>>>>>>>>> .../sampling/distribution/PoissonSampler.java   | 42
>>> ++++++++++++++++++++
>>>>>>>>>>> 1 file changed, 42 insertions(+)
>>>>>>>>>>> 
>>> ----------------------------------------------------------------------
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>> http://git-wip-us.apache.org/repos/asf/commons-rng/blob/f8159f28/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>>>>>>>>>>> 
>>> ----------------------------------------------------------------------
>>>>>>>>>>> diff --git
>>> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>>> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>>>>>>>>>>> index d0733ba..29d0e4e 100644
>>>>>>>>>>> ---
>>> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>>>>>>>>>>> +++
>>> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java
>>>>>>>>>>> @@ -67,6 +67,48 @@ public class PoissonSampler
>>>>>>>>>>>     return poissonSampler.sample();
>>>>>>>>>>> }
>>>>>>>>>>> 
>>>>>>>>>>> +    /**
>>>>>>>>>>> +     * @return a random value from a uniform distribution in the
>>>>>>>>>>> +     * interval {@code [0, 1)}.
>>>>>>>>>>> +     * @deprecated - one should be using the {@link
>>> PoissonSampler#sample()} method,
>>>>>>>>>>> +     * as it is considerably faster.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    @Deprecated
>>>>>>>>>>> +    protected double nextDouble() {
>>>>>>>>>>> +        return super.nextDouble();
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    /**
>>>>>>>>>>> +     * @return a random {@code int} value.
>>>>>>>>>>> +     * @deprecated - one should be using the {@link
>>> PoissonSampler#sample()} method,
>>>>>>>>>>> +     * as it is considerably faster.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    @Deprecated
>>>>>>>>>>> +    protected int nextInt() {
>>>>>>>>>>> +        return super.nextInt();
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    /**
>>>>>>>>>>> +     * @param max Upper bound (excluded).
>>>>>>>>>>> +     * @return a random {@code int} value in the interval
>>> {@code [0, max)}.
>>>>>>>>>>> +     * @deprecated - one should be using the {@link
>>> PoissonSampler#sample()} method,
>>>>>>>>>>> +     *      * as it is considerably faster.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    @Deprecated
>>>>>>>>>>> +    protected int nextInt(int max) {
>>>>>>>>>>> +        return super.nextInt(max);
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    /**
>>>>>>>>>>> +     * @return a random {@code long} value.
>>>>>>>>>>> +     * @deprecated - one should be using the {@link
>>> PoissonSampler#sample()} method,
>>>>>>>>>>> +     *      * as it is considerably faster.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    @Deprecated
>>>>>>>>>>> +    protected long nextLong() {
>>>>>>>>>>> +        return super.nextLong();
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> /** {@inheritDoc} */
>>>>>>>>>>> @Override
>>>>>>>>>>> public String toString() {
>>>>>>>>>>> 
>>>> 
>>>> 
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
>>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>>>> 
>>> 
>>> 
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>>> 
>>> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx