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. [...])


On Thu, 9 Aug 2018 08:49:26 -0600, Gary Gregory 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.

Are you kidding?
Here, "wrong usage" implies wrong results.

Wrong usage is worse than JAR hell.
[But should it occur in this case, it will manifest itself by
giving wrong results (if the stack happened to select v1.0) or
raising a "VerifyError" (with v1.1 with the proposed changes).]

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."

On the one hand, you declare that you cannot discuss the issue
but on the other, you think that you know what is the worse of
two evils.
What about trusting that those "close enough to this component"
do propose the best solution in the current situation?

If we make the (BC breaking) changes, then
 * either no stack will be broken,
 * or the user will be warned that his code is computing invalid
   results.
If we don't make the changes, then
 * no stack will be broken, but
 * the user may unsuspectingly continue getting invalid results.

IMO that's a pretty easy choice.

Gilles

P.S. As a side note, we should all see now why I insisted on
     having the "core" code (RNG algorithms) in one component
     and the "application" code (a.o. samplers) in another: all
     the issues, delays, workarounds (and all the changes for
     v1.1) solely concern module "commons-rng-sampling".
     And because this application needs fixing, other applications
     out there, even those that do *not* use the samplers, would
     be required to modify their source code?


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