[math] wrong backport commit in MATH_3_X ?

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[math] wrong backport commit in MATH_3_X ?

Rostislav Krasny
Hi there,

I've just noticed there could be a wrong backport commit done in MATH_3_X:

https://git1-us-west.apache.org/repos/asf?p=commons-math.git;a=commitdiff;h=c9c252bf26165e7fafd093cd892af35b23aa8f3f

This backport commit was discussed with Luc Maisonobe in this mailing
list in the thread about "releasing 3.6". Luc agreed that changes
introduced by MATH-1300, 1304 and 1305 could be backported into 3.6.

But this backport commit was done differently and probably by a wrong
way. It misses all changes in the following file:
src/main/java/org/apache/commons/math3/random/AbstractRandomGenerator.java

and probably some changes in this one too:
src/changes/changes.xml

it also seems changing the following file by a wrong way:
src/main/java/org/apache/commons/math3/random/BitsStreamGenerator.java

After this commit the BitsStreamGenerator has a new public nextBytes()
method that I asked to add in MATH-1306 into both BitsStreamGenerator
and AbstractRandomGenerator classes.

On the one hand I'm happy that BitsStreamGenerator was changed as I
proposed in MATH-1300, 1304, 1305 and finally 1306 and a few comments
in 1307. But why AbstractRandomGenerator was not changed by the same
way? It looks like Gilles had confused with changes of MATH-1307 and
1308 after which AbstractRandomGenerator has just disappeared in the
master branch.

I also thought the additional nextBytes() method will not be accepted
for the backport into 3.6 because it introduces an API change. This is
why I didn't ask Luc about MATH-1306. However I think this change is
fully backward compatible whith the previous MC 3.x releases. If you
think so as well and this additional nextBytes() method can stay in
3.6 too, I would be just happy. But let's change the
AbstractRandomGenerator accordingly. I also suggest to add "@since
3.6" into the javadoc of this new method in both classes.

Happy New Year

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [math] wrong backport commit in MATH_3_X ?

Gilles Sadowski
Hello.

On Fri, 1 Jan 2016 21:02:09 +0200, Rostislav Krasny wrote:

> Hi there,
>
> I've just noticed there could be a wrong backport commit done in
> MATH_3_X:
>
>
> https://git1-us-west.apache.org/repos/asf?p=commons-math.git;a=commitdiff;h=c9c252bf26165e7fafd093cd892af35b23aa8f3f
>
> This backport commit was discussed with Luc Maisonobe in this mailing
> list in the thread about "releasing 3.6". Luc agreed that changes
> introduced by MATH-1300, 1304 and 1305 could be backported into 3.6.
>
> But this backport commit was done differently and probably by a wrong
> way. It misses all changes in the following file:
>
> src/main/java/org/apache/commons/math3/random/AbstractRandomGenerator.java

That was not a mistake.

It was agreed that this class should be removed (MATH-1308).
It's pointless to put additional work into it. [IMO, it should have
been marked as deprecated to warn users to not rely on this anymore
(but, somehow, this is deemed not acceptable).]

> and probably some changes in this one too:
> src/changes/changes.xml
>
> it also seems changing the following file by a wrong way:
>
> src/main/java/org/apache/commons/math3/random/BitsStreamGenerator.java
>
> After this commit the BitsStreamGenerator has a new public
> nextBytes()
> method that I asked to add in MATH-1306 into both BitsStreamGenerator
> and AbstractRandomGenerator classes.
>
> On the one hand I'm happy that BitsStreamGenerator was changed as I
> proposed in MATH-1300, 1304, 1305 and finally 1306 and a few comments
> in 1307. But why AbstractRandomGenerator was not changed by the same
> way? It looks like Gilles had confused with changes of MATH-1307 and
> 1308 after which AbstractRandomGenerator has just disappeared in the
> master branch.
>
> I also thought the additional nextBytes() method will not be accepted
> for the backport into 3.6 because it introduces an API change.

Adding a method to a class is a compatible change.

However, now that I think of it, the modification (which you asked
to enable the feature discussed in MATH-1300) implies a change of
behaviour: Because of the removal of one call to "next(32)", some
sequences of numbers won't be the same between 3.5 and 3.6.

It can be construed that it's a bug fix.
But since "optimal performance" was not promised in the Javadoc, maybe
that it is not acceptable to sacrifice the "old" sequence for it.

> This is
> why I didn't ask Luc about MATH-1306. However I think this change is
> fully backward compatible whith the previous MC 3.x releases. If you
> think so as well and this additional nextBytes() method can stay in
> 3.6 too, I would be just happy. But let's change the
> AbstractRandomGenerator accordingly.

No.

> I also suggest to add "@since
> 3.6" into the javadoc of this new method in both classes.
>
> Happy New Year

Thanks,
Gilles


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]