On 12/21/2015 04:41 AM, Gilles wrote: > On Sat, 19 Dec 2015 11:35:26 -0700, Phil Steitz wrote: >> On 12/19/15 9:02 AM, Gilles wrote: >>> Hi. >>> >>> While experimenting on >>> https://issues.apache.org/jira/browse/MATH-1300 >>> I created a new >>> JDKRandomGeneratorTest >>> that inherits from >>> RandomGeneratorAbstractTest >>> similarly to the classes for testing all the other RNG implemented >>> in CM. >>> >>> The following tests (implemented in the base class) failed: >>> 1. >>> >>> testNextIntNeg(org.apache.commons.math4.random.JDKRandomGeneratorTest) >>> Time elapsed: 0.001 sec <<< ERROR! >>> java.lang.Exception: Unexpected exception, >>> >>> expected<org.apache.commons.math4.exception.MathIllegalArgumentException> >>> >>> but was<java.lang.IllegalArgumentException> >>> 2. >>> >>> testNextIntIAE2(org.apache.commons.math4.random.JDKRandomGeneratorTest) >>> Time elapsed: 0.015 sec <<< ERROR! >>> java.lang.IllegalArgumentException: bound must be positive >>> >>> This is caused by try/catch clauses that expect a >>> "MathIllegalArgumentException" >>> but "JDKRandomGenerator" extends "java.util.Random" that for those >>> methods throws >>> "IllegalArgumentException". >>> >>> What to do? >> >> I would change the test to expect IllegalArgumentException. Most >> [math] generators actually throw NotStrictlyPositiveException here, >> which extends MIAE, which extends IAE, so this should work. > > It turns out that, in the master branch, the hierarchy is > > RuntimeException > | > MathRuntimeException > | > MathIllegalArgumentException > > as per > https://issues.apache.org/jira/browse/MATH-853 > > [And the Javadoc and "throws" clauses are not yet consistent with this > in all the code base (e.g. the "RandomGenerator" interface).] > > So, in 4.0, "JDKRandomGenerator" should probably not inherit from > "java.util.Random" but delegate to it, trap standard exceptions raised, > and rethrow CM ones.
which is probably a good indication that the current situation in CM4 (as per MATH-853) was not a good design decision. I applied the changes as I thought the issue was settled, but it turns out that some of its implications were not fully taken into account. >From my POV, we should stick to the existing exceptions were applicable, as this is what people usually expect and is good practice. This means we should not create our own MathIAE but instead throw a standard IAE. I understand that the MIAE was created to support the localization of exception messages, but I wonder if this is really needed in that case. Usually, when an IAE is thrown it indicates a bug, as the developer did not provide proper arguments as indicated per javadoc. Now I do not see the value of being able to localize such exceptions as *only* developers should ever see them. For any other exceptions (typically converge exceptions or similar) which are clearly algorithm dependent, I fully support the design of 1 base MathRuntimeException with localization support. Thomas --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
