[Math] Exceptions from "JDKRandomGenerator"

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

[Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
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?


Regards,
Gilles


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Phil Steitz
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.

Phil

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


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
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.


Gilles


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Phil Steitz
On 12/20/15 8:41 PM, 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.

I guess that is the only way out if we are going to stick with the
MATH-853 setup.  This example illustrates a drawback identified in
the thread mentioned in the ticket.  I would personally be happy
reverting master back to the 3.x setup.

Phil

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


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:

> On 12/20/15 8:41 PM, 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.
>
> I guess that is the only way out if we are going to stick with the
> MATH-853 setup.  This example illustrates a drawback identified in
> the thread mentioned in the ticket.  I would personally be happy
> reverting master back to the 3.x setup.

We can't throw (!) the many discussions that led to MATH-853 by just
reverting on the basis of the current issue.

Perhaps that the multiple hierarchies are better, maybe not.
But we have to figure out arguments that are more convincing than
nostalgia.

For instance, IMO, we have contradicting wishes:
* Have CM exception inherits from the JDK ones with the same semantics.
* Have all CM exceptions provide the same additional functionality (the
   "ExceptionContext").

The first looks nice from a user POV (as in "I know this already").
The second is an internal requirement to avoid code duplication.

IMO, it always comes back to those opposite views we have concerning
the
place where human-readable messages should be generated: my view is
that
if a low-level library message bubbles up to the console, then it's not
our fault, but the application programmer's.

Please assume that for a moment; then CM could have its algorithms
throw
some internal subclass of "MathRuntimeException" (whose type is known
to
the caller) and the caller can decide whether he must trap it in order
to rethrow a standard type, or his own type (tailored to the context
which
he knows but CM doesn't).

In that scenario, it is much easier for the caller when the low-level
library's exceptions hierarchy is unique (especially when he uses
several
libraries).
And it's also easier for us because we can avoid code duplication.

You never convinced me that you were aware of this type of scenario and
why, somehow, it was trumped by a nicely formatted message on the
console.
The more so that a single hierarchy does not prevent the latter:
at the point where the message is printed, the exception type is
useless.

To summarize, if the top-level message matters most, then I'd be -1 to
revert
(because that's an orthogonal issue); if having standard types matters
most,
I'd wait for use-cases that show how these types are used in the
caller's
code before trying to figure out how to satisfy them (while taking our
internal requirements in to account too).

In conclusion, let's not jump to conclusions. ;-)


Gilles


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Phil Steitz
On 12/21/15 8:21 AM, Gilles wrote:

> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>> On 12/20/15 8:41 PM, 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.
>>
>> I guess that is the only way out if we are going to stick with the
>> MATH-853 setup.  This example illustrates a drawback identified in
>> the thread mentioned in the ticket.  I would personally be happy
>> reverting master back to the 3.x setup.
>
> We can't throw (!) the many discussions that led to MATH-853 by just
> reverting on the basis of the current issue.
>
> Perhaps that the multiple hierarchies are better, maybe not.
> But we have to figure out arguments that are more convincing than
> nostalgia.
>
> For instance, IMO, we have contradicting wishes:
> * Have CM exception inherits from the JDK ones with the same
> semantics.
> * Have all CM exceptions provide the same additional functionality
> (the
>   "ExceptionContext").

In 3.x, MIAE extends IAE and we have all the context stuff working.

I guess you are concerned about the "duplicated code" in the
multiple roots extending the standard exceptions.  Here is the full
extent of it:

 /**
     * @param pattern Message pattern explaining the cause of the error.
     * @param args Arguments.
     */
    public MathIllegalArgumentException(Localizable pattern,
                                        Object ... args) {
        context = new ExceptionContext(this);
        context.addMessage(pattern, args);
    }

    /** {@inheritDoc} */
    public ExceptionContext getContext() {
        return context;
    }

    /** {@inheritDoc} */
    @Override
    public String getMessage() {
        return context.getMessage();
    }

    /** {@inheritDoc} */
    @Override
    public String getLocalizedMessage() {
        return context.getLocalizedMessage();
    }

I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
this little bit of similar code.  That is after 10 years of
development.  I doubt we will have more than a few more needed at
the top level.

>
> The first looks nice from a user POV (as in "I know this already").
> The second is an internal requirement to avoid code duplication.
>
> IMO, it always comes back to those opposite views we have
> concerning the
> place where human-readable messages should be generated: my view
> is that
> if a low-level library message bubbles up to the console, then
> it's not
> our fault, but the application programmer's.
Meaningful exception messages are a best practice in Java.  They are
a very useful aid in debugging and production support.

>
> Please assume that for a moment; then CM could have its algorithms
> throw
> some internal subclass of "MathRuntimeException" (whose type is
> known to
> the caller) and the caller can decide whether he must trap it in
> order
> to rethrow a standard type, or his own type (tailored to the
> context which
> he knows but CM doesn't).
>
> In that scenario, it is much easier for the caller when the low-level
> library's exceptions hierarchy is unique (especially when he uses
> several
> libraries).
> And it's also easier for us because we can avoid code duplication.

Easiest is if the library follows the Java best practice, which is
to favor standard exceptions.  Then these can be caught without
rummaging through library-specific javadoc to figure out what
unchecked exception means what.  This is especially true for
unchecked exceptions.
>
> You never convinced me that you were aware of this type of
> scenario and
> why, somehow, it was trumped by a nicely formatted message on the
> console.
> The more so that a single hierarchy does not prevent the latter:
> at the point where the message is printed, the exception type is
> useless.

I think we can have it both ways.  The 3.x setup allows us to extend
standard exceptions *and* provide good exception messages.

>
> To summarize, if the top-level message matters most, then I'd be
> -1 to revert
> (because that's an orthogonal issue); if having standard types
> matters most,
> I'd wait for use-cases that show how these types are used in the
> caller's
> code before trying to figure out how to satisfy them (while taking
> our
> internal requirements in to account too).

Well, we have a nice example in front of us in our own test case
here.  But the bigger deal is the pain for every user who when
migrating from 3.x to 4 has to stop catching standard IAE if they
did this and replace it with MIAE.  Note that the compiler will be
no help there - failure to "comply" will cause the client app to "go
boom."

Phil

>
> In conclusion, let's not jump to conclusions. ;-)
>
>
> Gilles
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>



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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:

> On 12/21/15 8:21 AM, Gilles wrote:
>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>> On 12/20/15 8:41 PM, 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.
>>>
>>> I guess that is the only way out if we are going to stick with the
>>> MATH-853 setup.  This example illustrates a drawback identified in
>>> the thread mentioned in the ticket.  I would personally be happy
>>> reverting master back to the 3.x setup.
>>
>> We can't throw (!) the many discussions that led to MATH-853 by just
>> reverting on the basis of the current issue.
>>
>> Perhaps that the multiple hierarchies are better, maybe not.
>> But we have to figure out arguments that are more convincing than
>> nostalgia.
>>
>> For instance, IMO, we have contradicting wishes:
>> * Have CM exception inherits from the JDK ones with the same
>> semantics.
>> * Have all CM exceptions provide the same additional functionality
>> (the
>>   "ExceptionContext").
>
> In 3.x, MIAE extends IAE and we have all the context stuff working.
>
> I guess you are concerned about the "duplicated code" in the
> multiple roots extending the standard exceptions.  Here is the full
> extent of it:
>
>  /**
>      * @param pattern Message pattern explaining the cause of the
> error.
>      * @param args Arguments.
>      */
>     public MathIllegalArgumentException(Localizable pattern,
>                                         Object ... args) {
>         context = new ExceptionContext(this);
>         context.addMessage(pattern, args);
>     }
>
>     /** {@inheritDoc} */
>     public ExceptionContext getContext() {
>         return context;
>     }
>
>     /** {@inheritDoc} */
>     @Override
>     public String getMessage() {
>         return context.getMessage();
>     }
>
>     /** {@inheritDoc} */
>     @Override
>     public String getLocalizedMessage() {
>         return context.getLocalizedMessage();
>     }
>
> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
> this little bit of similar code.  That is after 10 years of
> development.  I doubt we will have more than a few more needed at
> the top level.

It's not a problem of how many lines of code must be duplicated.

The problem is the duplication.
Duplication is not a best practice AFAIK.

>> The first looks nice from a user POV (as in "I know this already").
>> The second is an internal requirement to avoid code duplication.
>>
>> IMO, it always comes back to those opposite views we have
>> concerning the
>> place where human-readable messages should be generated: my view
>> is that
>> if a low-level library message bubbles up to the console, then
>> it's not
>> our fault, but the application programmer's.
> Meaningful exception messages are a best practice in Java.  They are
> a very useful aid in debugging and production support.
>>
>> Please assume that for a moment; then CM could have its algorithms
>> throw
>> some internal subclass of "MathRuntimeException" (whose type is
>> known to
>> the caller) and the caller can decide whether he must trap it in
>> order
>> to rethrow a standard type, or his own type (tailored to the
>> context which
>> he knows but CM doesn't).
>>
>> In that scenario, it is much easier for the caller when the
>> low-level
>> library's exceptions hierarchy is unique (especially when he uses
>> several
>> libraries).
>> And it's also easier for us because we can avoid code duplication.
>
> Easiest is if the library follows the Java best practice, which is
> to favor standard exceptions.

We do not favour standard exceptions because we _have_ to define our
own.
And we have done this for various "good" reasons in line with other
best practices, subject to internal requirements.

> Then these can be caught without
> rummaging through library-specific javadoc to figure out what
> unchecked exception means what.  This is especially true for
> unchecked exceptions.

A standard exception does not mean anything specific.
So why bother with the "implementation detail"?

try {
   /** any code that deep down may or may not call CM */
} catch (RuntimeException e) {
   // Deal with it or rethrow.
}

will work, always.  User does not need to know anything to print
and read the meaningful message, only JDK's "RuntimeException" here.

The problem is _not_ there.

>> You never convinced me that you were aware of this type of
>> scenario and
>> why, somehow, it was trumped by a nicely formatted message on the
>> console.
>> The more so that a single hierarchy does not prevent the latter:
>> at the point where the message is printed, the exception type is
>> useless.
>
> I think we can have it both ways.  The 3.x setup allows us to extend
> standard exceptions *and* provide good exception messages.

The question is: Why extending standard exceptions?
We came to another answer in MATH-853.

So the problem cannot be solved as if it did not exist.

>>
>> To summarize, if the top-level message matters most, then I'd be
>> -1 to revert
>> (because that's an orthogonal issue); if having standard types
>> matters most,
>> I'd wait for use-cases that show how these types are used in the
>> caller's
>> code before trying to figure out how to satisfy them (while taking
>> our
>> internal requirements in to account too).
>
> Well, we have a nice example in front of us in our own test case
> here.  But the bigger deal is the pain for every user who when
> migrating from 3.x to 4 has to stop catching standard IAE if they
> did this and replace it with MIAE.  Note that the compiler will be
> no help there - failure to "comply" will cause the client app to "go
> boom."
>

All this has been said again and again.

I'd like that we do not take our own assumptions for granted
(be they the scenario which I described, or that compatibility
is the ultimate goal, even if the design is crap) but instead
that we could talk about what a live project CM can be, looking
to what can be done in possibly novel ways.

A new major version is an opportunity to legally break
compatibility in order to try new things.
Instead we have full-time discussions to avoid changing anything.

Users are never forced to follow development.

Gilles


> Phil
>>
>> In conclusion, let's not jump to conclusions. ;-)
>>
>>
>> Gilles


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Phil Steitz
On 12/21/15 11:16 AM, Gilles wrote:

> On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
>> On 12/21/15 8:21 AM, Gilles wrote:
>>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>>> On 12/20/15 8:41 PM, 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.
>>>>
>>>> I guess that is the only way out if we are going to stick with the
>>>> MATH-853 setup.  This example illustrates a drawback identified in
>>>> the thread mentioned in the ticket.  I would personally be happy
>>>> reverting master back to the 3.x setup.
>>>
>>> We can't throw (!) the many discussions that led to MATH-853 by
>>> just
>>> reverting on the basis of the current issue.
>>>
>>> Perhaps that the multiple hierarchies are better, maybe not.
>>> But we have to figure out arguments that are more convincing than
>>> nostalgia.
>>>
>>> For instance, IMO, we have contradicting wishes:
>>> * Have CM exception inherits from the JDK ones with the same
>>> semantics.
>>> * Have all CM exceptions provide the same additional functionality
>>> (the
>>>   "ExceptionContext").
>>
>> In 3.x, MIAE extends IAE and we have all the context stuff working.
>>
>> I guess you are concerned about the "duplicated code" in the
>> multiple roots extending the standard exceptions.  Here is the full
>> extent of it:
>>
>>  /**
>>      * @param pattern Message pattern explaining the cause of the
>> error.
>>      * @param args Arguments.
>>      */
>>     public MathIllegalArgumentException(Localizable pattern,
>>                                         Object ... args) {
>>         context = new ExceptionContext(this);
>>         context.addMessage(pattern, args);
>>     }
>>
>>     /** {@inheritDoc} */
>>     public ExceptionContext getContext() {
>>         return context;
>>     }
>>
>>     /** {@inheritDoc} */
>>     @Override
>>     public String getMessage() {
>>         return context.getMessage();
>>     }
>>
>>     /** {@inheritDoc} */
>>     @Override
>>     public String getLocalizedMessage() {
>>         return context.getLocalizedMessage();
>>     }
>>
>> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
>> this little bit of similar code.  That is after 10 years of
>> development.  I doubt we will have more than a few more needed at
>> the top level.
>
> It's not a problem of how many lines of code must be duplicated.
>
> The problem is the duplication.
> Duplication is not a best practice AFAIK.

I agree that duplication is to be avoided; but I think it does make
a difference how much / how complex the code is.  The duplicated
code in this case is trivial.

>
>>> The first looks nice from a user POV (as in "I know this already").
>>> The second is an internal requirement to avoid code duplication.
>>>
>>> IMO, it always comes back to those opposite views we have
>>> concerning the
>>> place where human-readable messages should be generated: my view
>>> is that
>>> if a low-level library message bubbles up to the console, then
>>> it's not
>>> our fault, but the application programmer's.
>> Meaningful exception messages are a best practice in Java.  They are
>> a very useful aid in debugging and production support.
>>>
>>> Please assume that for a moment; then CM could have its algorithms
>>> throw
>>> some internal subclass of "MathRuntimeException" (whose type is
>>> known to
>>> the caller) and the caller can decide whether he must trap it in
>>> order
>>> to rethrow a standard type, or his own type (tailored to the
>>> context which
>>> he knows but CM doesn't).
>>>
>>> In that scenario, it is much easier for the caller when the
>>> low-level
>>> library's exceptions hierarchy is unique (especially when he uses
>>> several
>>> libraries).
>>> And it's also easier for us because we can avoid code duplication.
>>
>> Easiest is if the library follows the Java best practice, which is
>> to favor standard exceptions.
>
> We do not favour standard exceptions because we _have_ to define our
> own.
> And we have done this for various "good" reasons in line with other
> best practices, subject to internal requirements.

We can have it both ways if we define our own exceptions to extend
the standard exceptions where that makes sense, which is the best
practice.  Having MIAE extend IAE is good design, IMO.  What we are
throwing on bad arguments is IAE.  Callers can catch that.  If they
want to dig in deeper and differentiate their handlers based on
which of our IAE subclasses is thrown, they can do that.  That is
how exception hierarchies are supposed to work.  The "favor standard
exceptions" principle is there so people don't go and define "new"
exceptions that are just renamed versions of the standard
exceptions.  That is precisely what MIAE does in V4.  The base MIAE
*is* IAE, just renamed, disconnecting all of the exceptions derived
from it from their natural root, which is IAE.

>
>> Then these can be caught without
>> rummaging through library-specific javadoc to figure out what
>> unchecked exception means what.  This is especially true for
>> unchecked exceptions.
>
> A standard exception does not mean anything specific.

It does.  That's the point.  IAE means you have provided bad
arguments, ISE means you have encountered bad state.
ArithmeticException means an exceptional condition has been
encountered performing an arithmetic operation.  We should, where
natural, derive from these.

> So why bother with the "implementation detail"?
>
> try {
>   /** any code that deep down may or may not call CM */
> } catch (RuntimeException e) {
>   // Deal with it or rethrow.
> }
>
> will work, always.  User does not need to know anything to print
> and read the meaningful message, only JDK's "RuntimeException" here.
>
> The problem is _not_ there.

Not sure I follow what you mean.

>
>>> You never convinced me that you were aware of this type of
>>> scenario and
>>> why, somehow, it was trumped by a nicely formatted message on the
>>> console.
>>> The more so that a single hierarchy does not prevent the latter:
>>> at the point where the message is printed, the exception type is
>>> useless.
>>
>> I think we can have it both ways.  The 3.x setup allows us to extend
>> standard exceptions *and* provide good exception messages.
>
> The question is: Why extending standard exceptions?
> We came to another answer in MATH-853.
>
> So the problem cannot be solved as if it did not exist.

I guess I am seeing more consequences now of completely proprietary
exceptions.   It bothered me then.  It bothers me more now as I
think about the consequences for client code.

>
>>>
>>> To summarize, if the top-level message matters most, then I'd be
>>> -1 to revert
>>> (because that's an orthogonal issue); if having standard types
>>> matters most,
>>> I'd wait for use-cases that show how these types are used in the
>>> caller's
>>> code before trying to figure out how to satisfy them (while taking
>>> our
>>> internal requirements in to account too).
>>
>> Well, we have a nice example in front of us in our own test case
>> here.  But the bigger deal is the pain for every user who when
>> migrating from 3.x to 4 has to stop catching standard IAE if they
>> did this and replace it with MIAE.  Note that the compiler will be
>> no help there - failure to "comply" will cause the client app to "go
>> boom."
>>
>
> All this has been said again and again.
>
> I'd like that we do not take our own assumptions for granted
> (be they the scenario which I described, or that compatibility
> is the ultimate goal, even if the design is crap) but instead
> that we could talk about what a live project CM can be, looking
> to what can be done in possibly novel ways.
>
> A new major version is an opportunity to legally break
> compatibility in order to try new things.
> Instead we have full-time discussions to avoid changing anything.

Not a fair statement.  I want to make sure that the changes we make
are positive.

Phil

>
> Users are never forced to follow development.
>
> Gilles
>
>
>> Phil
>>>
>>> In conclusion, let's not jump to conclusions. ;-)
>>>
>>>
>>> Gilles
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>



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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Luc Maisonobe-2
Hi all,

Le 21/12/2015 20:41, Phil Steitz a écrit :

> On 12/21/15 11:16 AM, Gilles wrote:
>> On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
>>> On 12/21/15 8:21 AM, Gilles wrote:
>>>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>>>> On 12/20/15 8:41 PM, 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.
>>>>>
>>>>> I guess that is the only way out if we are going to stick with the
>>>>> MATH-853 setup.  This example illustrates a drawback identified in
>>>>> the thread mentioned in the ticket.  I would personally be happy
>>>>> reverting master back to the 3.x setup.
>>>>
>>>> We can't throw (!) the many discussions that led to MATH-853 by
>>>> just
>>>> reverting on the basis of the current issue.
>>>>
>>>> Perhaps that the multiple hierarchies are better, maybe not.
>>>> But we have to figure out arguments that are more convincing than
>>>> nostalgia.
>>>>
>>>> For instance, IMO, we have contradicting wishes:
>>>> * Have CM exception inherits from the JDK ones with the same
>>>> semantics.
>>>> * Have all CM exceptions provide the same additional functionality
>>>> (the
>>>>   "ExceptionContext").
>>>
>>> In 3.x, MIAE extends IAE and we have all the context stuff working.
>>>
>>> I guess you are concerned about the "duplicated code" in the
>>> multiple roots extending the standard exceptions.  Here is the full
>>> extent of it:
>>>
>>>  /**
>>>      * @param pattern Message pattern explaining the cause of the
>>> error.
>>>      * @param args Arguments.
>>>      */
>>>     public MathIllegalArgumentException(Localizable pattern,
>>>                                         Object ... args) {
>>>         context = new ExceptionContext(this);
>>>         context.addMessage(pattern, args);
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     public ExceptionContext getContext() {
>>>         return context;
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     @Override
>>>     public String getMessage() {
>>>         return context.getMessage();
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     @Override
>>>     public String getLocalizedMessage() {
>>>         return context.getLocalizedMessage();
>>>     }
>>>
>>> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
>>> this little bit of similar code.  That is after 10 years of
>>> development.  I doubt we will have more than a few more needed at
>>> the top level.
>>
>> It's not a problem of how many lines of code must be duplicated.
>>
>> The problem is the duplication.
>> Duplication is not a best practice AFAIK.
>
> I agree that duplication is to be avoided; but I think it does make
> a difference how much / how complex the code is.  The duplicated
> code in this case is trivial.
>>
>>>> The first looks nice from a user POV (as in "I know this already").
>>>> The second is an internal requirement to avoid code duplication.
>>>>
>>>> IMO, it always comes back to those opposite views we have
>>>> concerning the
>>>> place where human-readable messages should be generated: my view
>>>> is that
>>>> if a low-level library message bubbles up to the console, then
>>>> it's not
>>>> our fault, but the application programmer's.
>>> Meaningful exception messages are a best practice in Java.  They are
>>> a very useful aid in debugging and production support.
>>>>
>>>> Please assume that for a moment; then CM could have its algorithms
>>>> throw
>>>> some internal subclass of "MathRuntimeException" (whose type is
>>>> known to
>>>> the caller) and the caller can decide whether he must trap it in
>>>> order
>>>> to rethrow a standard type, or his own type (tailored to the
>>>> context which
>>>> he knows but CM doesn't).
>>>>
>>>> In that scenario, it is much easier for the caller when the
>>>> low-level
>>>> library's exceptions hierarchy is unique (especially when he uses
>>>> several
>>>> libraries).
>>>> And it's also easier for us because we can avoid code duplication.
>>>
>>> Easiest is if the library follows the Java best practice, which is
>>> to favor standard exceptions.
>>
>> We do not favour standard exceptions because we _have_ to define our
>> own.
>> And we have done this for various "good" reasons in line with other
>> best practices, subject to internal requirements.
>
> We can have it both ways if we define our own exceptions to extend
> the standard exceptions where that makes sense, which is the best
> practice.  Having MIAE extend IAE is good design, IMO.  What we are
> throwing on bad arguments is IAE.  Callers can catch that.  If they
> want to dig in deeper and differentiate their handlers based on
> which of our IAE subclasses is thrown, they can do that.  That is
> how exception hierarchies are supposed to work.  The "favor standard
> exceptions" principle is there so people don't go and define "new"
> exceptions that are just renamed versions of the standard
> exceptions.  That is precisely what MIAE does in V4.  The base MIAE
> *is* IAE, just renamed, disconnecting all of the exceptions derived
> from it from their natural root, which is IAE.

One of the point in having exceptions that extends our own root
exception is that users at higher level can catch this top level.
Currently, we don't even advertise properly what we throw. We even
miss to forward upward some exceptions thrown at low level in the
javadoc/signature of out upper level methods.

So user may currently not know, only reading the javadoc/signature
of one of our implementation that they may get a MIAE or something
else. If we were using a single root, they would at least be able
to do a catch (MathRootException) that would prevent a runtime
exception to bubble up too far. Currently, defensive programming
to protect against this failure is to catch all of
MathArithmeticException, MathIllegalArgumentException,
MathIllegalNumberException, MathIllegalStateException,
MathUnsupportedOperationException, and MathRuntimeException.

In a perfect world, we would be able to extend a regular IAE while
implementing a MathRootException, but Throwable in Java is a class,
not an interface. Too bad.

best regards,
Luc

>
>>
>>> Then these can be caught without
>>> rummaging through library-specific javadoc to figure out what
>>> unchecked exception means what.  This is especially true for
>>> unchecked exceptions.
>>
>> A standard exception does not mean anything specific.
>
> It does.  That's the point.  IAE means you have provided bad
> arguments, ISE means you have encountered bad state.
> ArithmeticException means an exceptional condition has been
> encountered performing an arithmetic operation.  We should, where
> natural, derive from these.
>> So why bother with the "implementation detail"?
>>
>> try {
>>   /** any code that deep down may or may not call CM */
>> } catch (RuntimeException e) {
>>   // Deal with it or rethrow.
>> }
>>
>> will work, always.  User does not need to know anything to print
>> and read the meaningful message, only JDK's "RuntimeException" here.
>>
>> The problem is _not_ there.
>
> Not sure I follow what you mean.
>>
>>>> You never convinced me that you were aware of this type of
>>>> scenario and
>>>> why, somehow, it was trumped by a nicely formatted message on the
>>>> console.
>>>> The more so that a single hierarchy does not prevent the latter:
>>>> at the point where the message is printed, the exception type is
>>>> useless.
>>>
>>> I think we can have it both ways.  The 3.x setup allows us to extend
>>> standard exceptions *and* provide good exception messages.
>>
>> The question is: Why extending standard exceptions?
>> We came to another answer in MATH-853.
>>
>> So the problem cannot be solved as if it did not exist.
>
> I guess I am seeing more consequences now of completely proprietary
> exceptions.   It bothered me then.  It bothers me more now as I
> think about the consequences for client code.
>
>>
>>>>
>>>> To summarize, if the top-level message matters most, then I'd be
>>>> -1 to revert
>>>> (because that's an orthogonal issue); if having standard types
>>>> matters most,
>>>> I'd wait for use-cases that show how these types are used in the
>>>> caller's
>>>> code before trying to figure out how to satisfy them (while taking
>>>> our
>>>> internal requirements in to account too).
>>>
>>> Well, we have a nice example in front of us in our own test case
>>> here.  But the bigger deal is the pain for every user who when
>>> migrating from 3.x to 4 has to stop catching standard IAE if they
>>> did this and replace it with MIAE.  Note that the compiler will be
>>> no help there - failure to "comply" will cause the client app to "go
>>> boom."
>>>
>>
>> All this has been said again and again.
>>
>> I'd like that we do not take our own assumptions for granted
>> (be they the scenario which I described, or that compatibility
>> is the ultimate goal, even if the design is crap) but instead
>> that we could talk about what a live project CM can be, looking
>> to what can be done in possibly novel ways.
>>
>> A new major version is an opportunity to legally break
>> compatibility in order to try new things.
>> Instead we have full-time discussions to avoid changing anything.
>
> Not a fair statement.  I want to make sure that the changes we make
> are positive.
>
> Phil
>>
>> Users are never forced to follow development.
>>
>> Gilles
>>
>>
>>> Phil
>>>>
>>>> In conclusion, let's not jump to conclusions. ;-)
>>>>
>>>>
>>>> Gilles
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

ole ersoy

.

> One of the point in having exceptions that extends our own root
> exception is that users at higher level can catch this top level.
> Currently, we don't even advertise properly what we throw. We even
> miss to forward upward some exceptions thrown at low level in the
> javadoc/signature of out upper level methods.
>
> So user may currently not know, only reading the javadoc/signature
> of one of our implementation that they may get a MIAE or something
> else. If we were using a single root,
Or just a single MathException.
>   they would at least be able
> to do a catch (MathRootException) that would prevent a runtime
> exception to bubble up too far. Currently, defensive programming
> to protect against this failure is to catch all of
> MathArithmeticException, MathIllegalArgumentException,
> MathIllegalNumberException, MathIllegalStateException,
> MathUnsupportedOperationException, and MathRuntimeException.
With the design I proposed, and the design I'm using, we only have to catch one.  After it's caught the type code (Enum) indicates precisely what the issue is.
>
> In a perfect world, we would be able to extend a regular IAE while
> implementing a MathRootException, but Throwable in Java is a class,
> not an interface. Too bad.
Luc - how do you feel about a single MathException that extends RuntimeException with an Enum that indicates what the root cause is and can be used as the key to look up the corresponding message template which can be resolved into a message using parameters attached to the MathException context?

Here's an example from my refactoring of ArithmeticUtils:

https://github.com/firefly-math/firefly-math-arithmetic/blob/master/src/main/java/com/fireflysemantics/math/arithmetic/Arithmetic.java

     /**
      * Add two integers, checking for overflow.
      *
      * @param x
      *            an addend
      * @param y
      *            an addend
      * @return the sum {@code x+y}
      * @throws MathException
      *             Of type {@code MAE__OVERFLOW_IN_ADDITION} if the result can
      *             not be represented as an {@code int}.
      */
     public static int addAndCheck(int x, int y) throws MathException {
         long s = (long) x
                 + (long) y;
         if (s < Integer.MIN_VALUE
                 || s > Integer.MAX_VALUE) {
             throw new MathException(MAE__OVERFLOW_IN_ADDITION).put(X, x).put(Y, y);
         }
         return (int) s;
     }

The toString() method of the exception is implemented like this and delivers the exception root cause (Enum) and parameter name and value pairs:

     @Override
     public String toString() {
         String parameters = context.entrySet().stream().map(e -> e.getKey()
                 + "="
                 + e.getValue()).collect(Collectors.joining(", "));

         return "Firefly math exception type "
                 + this.type
                 + ".  Context ["
                 + parameters
                 + "]";
     }

So we get a pretty good indication of what the issue is by just using toString() to construct the message.

Cheers,
Ole


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Luc Maisonobe-2
Le 22/12/2015 17:44, Ole Ersoy a écrit :

>
> .
>
>> One of the point in having exceptions that extends our own root
>> exception is that users at higher level can catch this top level.
>> Currently, we don't even advertise properly what we throw. We even
>> miss to forward upward some exceptions thrown at low level in the
>> javadoc/signature of out upper level methods.
>>
>> So user may currently not know, only reading the javadoc/signature
>> of one of our implementation that they may get a MIAE or something
>> else. If we were using a single root,
> Or just a single MathException.
>>   they would at least be able
>> to do a catch (MathRootException) that would prevent a runtime
>> exception to bubble up too far. Currently, defensive programming
>> to protect against this failure is to catch all of
>> MathArithmeticException, MathIllegalArgumentException,
>> MathIllegalNumberException, MathIllegalStateException,
>> MathUnsupportedOperationException, and MathRuntimeException.
> With the design I proposed, and the design I'm using, we only have to
> catch one.  After it's caught the type code (Enum) indicates precisely
> what the issue is.
>>
>> In a perfect world, we would be able to extend a regular IAE while
>> implementing a MathRootException, but Throwable in Java is a class,
>> not an interface. Too bad.
> Luc - how do you feel about a single MathException that extends
> RuntimeException with an Enum that indicates what the root cause is and
> can be used as the key to look up the corresponding message template
> which can be resolved into a message using parameters attached to the
> MathException context?

I am fine with this.

I did not check your design yet, sorry I am overwhelmed with several
issues to fix for 3.6 that I would like to release soon.

best regards
Luc

>
> Here's an example from my refactoring of ArithmeticUtils:
>
> https://github.com/firefly-math/firefly-math-arithmetic/blob/master/src/main/java/com/fireflysemantics/math/arithmetic/Arithmetic.java
>
>
>     /**
>      * Add two integers, checking for overflow.
>      *
>      * @param x
>      *            an addend
>      * @param y
>      *            an addend
>      * @return the sum {@code x+y}
>      * @throws MathException
>      *             Of type {@code MAE__OVERFLOW_IN_ADDITION} if the
> result can
>      *             not be represented as an {@code int}.
>      */
>     public static int addAndCheck(int x, int y) throws MathException {
>         long s = (long) x
>                 + (long) y;
>         if (s < Integer.MIN_VALUE
>                 || s > Integer.MAX_VALUE) {
>             throw new MathException(MAE__OVERFLOW_IN_ADDITION).put(X,
> x).put(Y, y);
>         }
>         return (int) s;
>     }
>
> The toString() method of the exception is implemented like this and
> delivers the exception root cause (Enum) and parameter name and value
> pairs:
>
>     @Override
>     public String toString() {
>         String parameters = context.entrySet().stream().map(e -> e.getKey()
>                 + "="
>                 + e.getValue()).collect(Collectors.joining(", "));
>
>         return "Firefly math exception type "
>                 + this.type
>                 + ".  Context ["
>                 + parameters
>                 + "]";
>     }
>
> So we get a pretty good indication of what the issue is by just using
> toString() to construct the message.
>
> Cheers,
> Ole
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Phil Steitz
In reply to this post by Luc Maisonobe-2
On 12/22/15 1:54 AM, Luc Maisonobe wrote:

> Hi all,
>
> Le 21/12/2015 20:41, Phil Steitz a écrit :
>> On 12/21/15 11:16 AM, Gilles wrote:
>>> On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
>>>> On 12/21/15 8:21 AM, Gilles wrote:
>>>>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>>>>> On 12/20/15 8:41 PM, 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.
>>>>>> I guess that is the only way out if we are going to stick with the
>>>>>> MATH-853 setup.  This example illustrates a drawback identified in
>>>>>> the thread mentioned in the ticket.  I would personally be happy
>>>>>> reverting master back to the 3.x setup.
>>>>> We can't throw (!) the many discussions that led to MATH-853 by
>>>>> just
>>>>> reverting on the basis of the current issue.
>>>>>
>>>>> Perhaps that the multiple hierarchies are better, maybe not.
>>>>> But we have to figure out arguments that are more convincing than
>>>>> nostalgia.
>>>>>
>>>>> For instance, IMO, we have contradicting wishes:
>>>>> * Have CM exception inherits from the JDK ones with the same
>>>>> semantics.
>>>>> * Have all CM exceptions provide the same additional functionality
>>>>> (the
>>>>>   "ExceptionContext").
>>>> In 3.x, MIAE extends IAE and we have all the context stuff working.
>>>>
>>>> I guess you are concerned about the "duplicated code" in the
>>>> multiple roots extending the standard exceptions.  Here is the full
>>>> extent of it:
>>>>
>>>>  /**
>>>>      * @param pattern Message pattern explaining the cause of the
>>>> error.
>>>>      * @param args Arguments.
>>>>      */
>>>>     public MathIllegalArgumentException(Localizable pattern,
>>>>                                         Object ... args) {
>>>>         context = new ExceptionContext(this);
>>>>         context.addMessage(pattern, args);
>>>>     }
>>>>
>>>>     /** {@inheritDoc} */
>>>>     public ExceptionContext getContext() {
>>>>         return context;
>>>>     }
>>>>
>>>>     /** {@inheritDoc} */
>>>>     @Override
>>>>     public String getMessage() {
>>>>         return context.getMessage();
>>>>     }
>>>>
>>>>     /** {@inheritDoc} */
>>>>     @Override
>>>>     public String getLocalizedMessage() {
>>>>         return context.getLocalizedMessage();
>>>>     }
>>>>
>>>> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
>>>> this little bit of similar code.  That is after 10 years of
>>>> development.  I doubt we will have more than a few more needed at
>>>> the top level.
>>> It's not a problem of how many lines of code must be duplicated.
>>>
>>> The problem is the duplication.
>>> Duplication is not a best practice AFAIK.
>> I agree that duplication is to be avoided; but I think it does make
>> a difference how much / how complex the code is.  The duplicated
>> code in this case is trivial.
>>>>> The first looks nice from a user POV (as in "I know this already").
>>>>> The second is an internal requirement to avoid code duplication.
>>>>>
>>>>> IMO, it always comes back to those opposite views we have
>>>>> concerning the
>>>>> place where human-readable messages should be generated: my view
>>>>> is that
>>>>> if a low-level library message bubbles up to the console, then
>>>>> it's not
>>>>> our fault, but the application programmer's.
>>>> Meaningful exception messages are a best practice in Java.  They are
>>>> a very useful aid in debugging and production support.
>>>>> Please assume that for a moment; then CM could have its algorithms
>>>>> throw
>>>>> some internal subclass of "MathRuntimeException" (whose type is
>>>>> known to
>>>>> the caller) and the caller can decide whether he must trap it in
>>>>> order
>>>>> to rethrow a standard type, or his own type (tailored to the
>>>>> context which
>>>>> he knows but CM doesn't).
>>>>>
>>>>> In that scenario, it is much easier for the caller when the
>>>>> low-level
>>>>> library's exceptions hierarchy is unique (especially when he uses
>>>>> several
>>>>> libraries).
>>>>> And it's also easier for us because we can avoid code duplication.
>>>> Easiest is if the library follows the Java best practice, which is
>>>> to favor standard exceptions.
>>> We do not favour standard exceptions because we _have_ to define our
>>> own.
>>> And we have done this for various "good" reasons in line with other
>>> best practices, subject to internal requirements.
>> We can have it both ways if we define our own exceptions to extend
>> the standard exceptions where that makes sense, which is the best
>> practice.  Having MIAE extend IAE is good design, IMO.  What we are
>> throwing on bad arguments is IAE.  Callers can catch that.  If they
>> want to dig in deeper and differentiate their handlers based on
>> which of our IAE subclasses is thrown, they can do that.  That is
>> how exception hierarchies are supposed to work.  The "favor standard
>> exceptions" principle is there so people don't go and define "new"
>> exceptions that are just renamed versions of the standard
>> exceptions.  That is precisely what MIAE does in V4.  The base MIAE
>> *is* IAE, just renamed, disconnecting all of the exceptions derived
>> from it from their natural root, which is IAE.
> One of the point in having exceptions that extends our own root
> exception is that users at higher level can catch this top level.
> Currently, we don't even advertise properly what we throw. We even
> miss to forward upward some exceptions thrown at low level in the
> javadoc/signature of out upper level methods.

This is bad.  We should keep chipping away at fixing it.
>
> So user may currently not know, only reading the javadoc/signature
> of one of our implementation that they may get a MIAE or something
> else. If we were using a single root, they would at least be able
> to do a catch (MathRootException) that would prevent a runtime
> exception to bubble up too far.

Not much different from "catch Exception" except I guess it will
catch only what is thrown by "us."
>  Currently, defensive programming
> to protect against this failure is to catch all of
> MathArithmeticException, MathIllegalArgumentException,
> MathIllegalNumberException, MathIllegalStateException,
> MathUnsupportedOperationException, and MathRuntimeException.
>
> In a perfect world, we would be able to extend a regular IAE while
> implementing a MathRootException, but Throwable in Java is a class,
> not an interface. Too bad.

Sorry for reopening this can of worms.  I see I am in the minority.
Apologies for the noise.

Phil

>
> best regards,
> Luc
>
>>>> Then these can be caught without
>>>> rummaging through library-specific javadoc to figure out what
>>>> unchecked exception means what.  This is especially true for
>>>> unchecked exceptions.
>>> A standard exception does not mean anything specific.
>> It does.  That's the point.  IAE means you have provided bad
>> arguments, ISE means you have encountered bad state.
>> ArithmeticException means an exceptional condition has been
>> encountered performing an arithmetic operation.  We should, where
>> natural, derive from these.
>>> So why bother with the "implementation detail"?
>>>
>>> try {
>>>   /** any code that deep down may or may not call CM */
>>> } catch (RuntimeException e) {
>>>   // Deal with it or rethrow.
>>> }
>>>
>>> will work, always.  User does not need to know anything to print
>>> and read the meaningful message, only JDK's "RuntimeException" here.
>>>
>>> The problem is _not_ there.
>> Not sure I follow what you mean.
>>>>> You never convinced me that you were aware of this type of
>>>>> scenario and
>>>>> why, somehow, it was trumped by a nicely formatted message on the
>>>>> console.
>>>>> The more so that a single hierarchy does not prevent the latter:
>>>>> at the point where the message is printed, the exception type is
>>>>> useless.
>>>> I think we can have it both ways.  The 3.x setup allows us to extend
>>>> standard exceptions *and* provide good exception messages.
>>> The question is: Why extending standard exceptions?
>>> We came to another answer in MATH-853.
>>>
>>> So the problem cannot be solved as if it did not exist.
>> I guess I am seeing more consequences now of completely proprietary
>> exceptions.   It bothered me then.  It bothers me more now as I
>> think about the consequences for client code.
>>
>>>>> To summarize, if the top-level message matters most, then I'd be
>>>>> -1 to revert
>>>>> (because that's an orthogonal issue); if having standard types
>>>>> matters most,
>>>>> I'd wait for use-cases that show how these types are used in the
>>>>> caller's
>>>>> code before trying to figure out how to satisfy them (while taking
>>>>> our
>>>>> internal requirements in to account too).
>>>> Well, we have a nice example in front of us in our own test case
>>>> here.  But the bigger deal is the pain for every user who when
>>>> migrating from 3.x to 4 has to stop catching standard IAE if they
>>>> did this and replace it with MIAE.  Note that the compiler will be
>>>> no help there - failure to "comply" will cause the client app to "go
>>>> boom."
>>>>
>>> All this has been said again and again.
>>>
>>> I'd like that we do not take our own assumptions for granted
>>> (be they the scenario which I described, or that compatibility
>>> is the ultimate goal, even if the design is crap) but instead
>>> that we could talk about what a live project CM can be, looking
>>> to what can be done in possibly novel ways.
>>>
>>> A new major version is an opportunity to legally break
>>> compatibility in order to try new things.
>>> Instead we have full-time discussions to avoid changing anything.
>> Not a fair statement.  I want to make sure that the changes we make
>> are positive.
>>
>> Phil
>>> Users are never forced to follow development.
>>>
>>> Gilles
>>>
>>>
>>>> Phil
>>>>> In conclusion, let's not jump to conclusions. ;-)
>>>>>
>>>>>
>>>>> Gilles
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
In reply to this post by Phil Steitz
On Mon, 21 Dec 2015 12:41:01 -0700, Phil Steitz wrote:

> On 12/21/15 11:16 AM, Gilles wrote:
>> On Mon, 21 Dec 2015 10:01:33 -0700, Phil Steitz wrote:
>>> On 12/21/15 8:21 AM, Gilles wrote:
>>>> On Mon, 21 Dec 2015 06:06:16 -0700, Phil Steitz wrote:
>>>>> On 12/20/15 8:41 PM, 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.
>>>>>
>>>>> I guess that is the only way out if we are going to stick with
>>>>> the
>>>>> MATH-853 setup.  This example illustrates a drawback identified
>>>>> in
>>>>> the thread mentioned in the ticket.  I would personally be happy
>>>>> reverting master back to the 3.x setup.
>>>>
>>>> We can't throw (!) the many discussions that led to MATH-853 by
>>>> just
>>>> reverting on the basis of the current issue.
>>>>
>>>> Perhaps that the multiple hierarchies are better, maybe not.
>>>> But we have to figure out arguments that are more convincing than
>>>> nostalgia.
>>>>
>>>> For instance, IMO, we have contradicting wishes:
>>>> * Have CM exception inherits from the JDK ones with the same
>>>> semantics.
>>>> * Have all CM exceptions provide the same additional functionality
>>>> (the
>>>>   "ExceptionContext").
>>>
>>> In 3.x, MIAE extends IAE and we have all the context stuff working.
>>>
>>> I guess you are concerned about the "duplicated code" in the
>>> multiple roots extending the standard exceptions.  Here is the full
>>> extent of it:
>>>
>>>  /**
>>>      * @param pattern Message pattern explaining the cause of the
>>> error.
>>>      * @param args Arguments.
>>>      */
>>>     public MathIllegalArgumentException(Localizable pattern,
>>>                                         Object ... args) {
>>>         context = new ExceptionContext(this);
>>>         context.addMessage(pattern, args);
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     public ExceptionContext getContext() {
>>>         return context;
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     @Override
>>>     public String getMessage() {
>>>         return context.getMessage();
>>>     }
>>>
>>>     /** {@inheritDoc} */
>>>     @Override
>>>     public String getLocalizedMessage() {
>>>         return context.getLocalizedMessage();
>>>     }
>>>
>>> I see that as NOT_A_PROBLEM.  In 3.x, we have 6 classes that need
>>> this little bit of similar code.  That is after 10 years of
>>> development.  I doubt we will have more than a few more needed at
>>> the top level.
>>
>> It's not a problem of how many lines of code must be duplicated.
>>
>> The problem is the duplication.
>> Duplication is not a best practice AFAIK.
>
> I agree that duplication is to be avoided; but I think it does make
> a difference how much / how complex the code is.  The duplicated
> code in this case is trivial.

Code duplication is a sure sign that the design must be questioned,
and that a better alternative probably exists.

>>
>>>> The first looks nice from a user POV (as in "I know this
>>>> already").
>>>> The second is an internal requirement to avoid code duplication.
>>>>
>>>> IMO, it always comes back to those opposite views we have
>>>> concerning the
>>>> place where human-readable messages should be generated: my view
>>>> is that
>>>> if a low-level library message bubbles up to the console, then
>>>> it's not
>>>> our fault, but the application programmer's.
>>> Meaningful exception messages are a best practice in Java.  They
>>> are
>>> a very useful aid in debugging and production support.
>>>>
>>>> Please assume that for a moment; then CM could have its algorithms
>>>> throw
>>>> some internal subclass of "MathRuntimeException" (whose type is
>>>> known to
>>>> the caller) and the caller can decide whether he must trap it in
>>>> order
>>>> to rethrow a standard type, or his own type (tailored to the
>>>> context which
>>>> he knows but CM doesn't).
>>>>
>>>> In that scenario, it is much easier for the caller when the
>>>> low-level
>>>> library's exceptions hierarchy is unique (especially when he uses
>>>> several
>>>> libraries).
>>>> And it's also easier for us because we can avoid code duplication.
>>>
>>> Easiest is if the library follows the Java best practice, which is
>>> to favor standard exceptions.
>>
>> We do not favour standard exceptions because we _have_ to define our
>> own.
>> And we have done this for various "good" reasons in line with other
>> best practices, subject to internal requirements.
>
> We can have it both ways if we define our own exceptions to extend
> the standard exceptions where that makes sense, which is the best
> practice.  Having MIAE extend IAE is good design, IMO.  What we are
> throwing on bad arguments is IAE.  Callers can catch that.  If they
> want to dig in deeper and differentiate their handlers based on
> which of our IAE subclasses is thrown, they can do that.  That is
> how exception hierarchies are supposed to work.  The "favor standard
> exceptions" principle is there so people don't go and define "new"
> exceptions that are just renamed versions of the standard
> exceptions.  That is precisely what MIAE does in V4.  The base MIAE
> *is* IAE, just renamed, disconnecting all of the exceptions derived
> from it from their natural root, which is IAE.

I agree that reuse is better.
As Luc said, we would have reused standard exceptions if they were
like interfaces.
There is a technical (creating duplicate code), because of how
inheritance works in Java, so I don't think that you can consider it
obvious that inheritance must be the way to go.

>>> Then these can be caught without
>>> rummaging through library-specific javadoc to figure out what
>>> unchecked exception means what.  This is especially true for
>>> unchecked exceptions.
>>
>> A standard exception does not mean anything specific.
>
> It does.  That's the point.  IAE means you have provided bad
> arguments, ISE means you have encountered bad state.
> ArithmeticException means an exceptional condition has been
> encountered performing an arithmetic operation.  We should, where
> natural, derive from these.

They are non-specific in the sense which you usually use when you
say that CM must provide a _detailed_ report of the failure.

If the details are provided by the message, the type of the exception
is not that important (compared to the cleanliness of the code for
creating of the message).
If the failure's cause is conveyed by exception type, then it is more
important to reuse existing types.

>> So why bother with the "implementation detail"?
>>
>> try {
>>   /** any code that deep down may or may not call CM */
>> } catch (RuntimeException e) {
>>   // Deal with it or rethrow.
>> }
>>
>> will work, always.  User does not need to know anything to print
>> and read the meaningful message, only JDK's "RuntimeException" here.
>>
>> The problem is _not_ there.
>
> Not sure I follow what you mean.

I tried to make it clearer in the previous paragraph.

>>
>>>> You never convinced me that you were aware of this type of
>>>> scenario and
>>>> why, somehow, it was trumped by a nicely formatted message on the
>>>> console.
>>>> The more so that a single hierarchy does not prevent the latter:
>>>> at the point where the message is printed, the exception type is
>>>> useless.
>>>
>>> I think we can have it both ways.  The 3.x setup allows us to
>>> extend
>>> standard exceptions *and* provide good exception messages.
>>
>> The question is: Why extending standard exceptions?
>> We came to another answer in MATH-853.
>>
>> So the problem cannot be solved as if it did not exist.
>
> I guess I am seeing more consequences now of completely proprietary
> exceptions.   It bothered me then.  It bothers me more now as I
> think about the consequences for client code.

As indicated by Ole in the other, I think that it makes sense for
an application/library to shield its callers from the possibly many
different exception types used by the libraries it uses.

It implies that even if CM were to throw standard exception instances
where appropriate according to the standard exception semantics, an
application's developer keen to provide a uniform interface to its
users
will wrap them into his own application-specific exceptions (that
could,
or not, extend the standard exceptions).

The CM developers seem strongly biased by the fact that they are also
those users of CM satisfied with letting the CM exception types bubble
up to the top.
IOW, the low level is coded according to what we like to see at the
top level. But how did we conclude that all developers would be fine
with a simple text message?
For example, if they want to restart a process depending on the cause
of the failure, it is much easier to examine the exception type rather
than parse the text message (the more so if it is localized!).

I must have mentioned this more than a few times along the years. :-}

>>>>
>>>> To summarize, if the top-level message matters most, then I'd be
>>>> -1 to revert
>>>> (because that's an orthogonal issue); if having standard types
>>>> matters most,
>>>> I'd wait for use-cases that show how these types are used in the
>>>> caller's
>>>> code before trying to figure out how to satisfy them (while taking
>>>> our
>>>> internal requirements in to account too).
>>>
>>> Well, we have a nice example in front of us in our own test case
>>> here.  But the bigger deal is the pain for every user who when
>>> migrating from 3.x to 4 has to stop catching standard IAE if they
>>> did this and replace it with MIAE.  Note that the compiler will be
>>> no help there - failure to "comply" will cause the client app to
>>> "go
>>> boom."
>>>
>>
>> All this has been said again and again.
>>
>> I'd like that we do not take our own assumptions for granted
>> (be they the scenario which I described, or that compatibility
>> is the ultimate goal, even if the design is crap) but instead
>> that we could talk about what a live project CM can be, looking
>> to what can be done in possibly novel ways.
>>
>> A new major version is an opportunity to legally break
>> compatibility in order to try new things.
>> Instead we have full-time discussions to avoid changing anything.
>
> Not a fair statement.  I want to make sure that the changes we make
> are positive.

I don't doubt it.
But I think that we have the right to be wrong while trying to make
a better library.

What is not fair is your statement that "client app [will go] boom":
It is not true, because they can continue using 3.x if they are not
willing to deal with a new library.

The only people annoyed are those who decide that they do not want
to use legacy code, but also do not want to deal with the consequences
of their _own_ decision.

Of course, there is the problem of the resources needed to maintain
legacy versions of CM.
But it is not fair that this should prevent compatibility-breaking
changes forever. It would be fair that those users who need legacy
code dedicate some of their time to its maintenance.

As Ole proposed, it might worth be splitting CM into modules that
could be release separately. Those modules on which few others
depend could then be released more often.

Gilles

>
> Phil
>>
>> Users are never forced to follow development.
>>
>> Gilles
>>
>>
>>> Phil
>>>>
>>>> In conclusion, let's not jump to conclusions. ;-)
>>>>
>>>>
>>>> Gilles


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Thomas Neidhart
In reply to this post by Gilles Sadowski
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
On Wed, 23 Dec 2015 16:26:52 +0100, Thomas Neidhart wrote:

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

It was consensual.
What you express below is far more controversial.

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

This is a point I made a long time ago (not "in a far, far away
galaxy").
To which I got the answer that CM must provide a
  * detailed,
  * localizable,
  * textual
error message.

IMO, for a bug pointed to by an IAE, all the developer has to know is
the
stack trace.

If we want to be "standard", we shouldn't even have to check for null
or
array length on caller's input data as we know that the JVM will do the
checks and trivially throw standard exceptions on these programming
errors!

Javadoc and stack trace are necessary and sufficient to fix those bugs.

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

This is a really tiny subset. Ole's proposal could focus on those few
cases.
For the rest, I'm all for being consistently "standard"; meaning that
in CM we'd write code similar to the following:

/**
  * Comment on "evaluate".
  *
  * @paran n Comment on "n".
  * @return the result.
  * @throws IllegalArgumentException if {@code n <= 0}.
  */
public double evaluate(int n) {
   if (n <= 0) {
     throw new IllegalArgumentException("n <= 0");
   }

   return compute(n);
}

KISS.

And if somewhere else there is
   throw new IllegalArgumentException("p <= 0");
I won't consider it duplicate code...


Gilles

>
> Thomas


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Thomas Neidhart
On 12/23/2015 05:39 PM, Gilles wrote:

> On Wed, 23 Dec 2015 16:26:52 +0100, Thomas Neidhart wrote:
>> 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.
>
> It was consensual.
> What you express below is far more controversial.
>
>> 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.
>
> This is a point I made a long time ago (not "in a far, far away galaxy").
> To which I got the answer that CM must provide a
>  * detailed,
>  * localizable,
>  * textual
> error message.
>
> IMO, for a bug pointed to by an IAE, all the developer has to know is the
> stack trace.
>
> If we want to be "standard", we shouldn't even have to check for null or
> array length on caller's input data as we know that the JVM will do the
> checks and trivially throw standard exceptions on these programming errors!

Checking input parameters and explicitly throwing exceptions *is*
considered to be good practice.

Some of the commons libraries that exist for a very long time do it
differently (especially lang and collections), but this might lead to
inconsistent behavior (check the numerous bug reports related to that)
depending on input. I understand that 15 years ago this was not yet good
practice, but it is nowadays imho.

> Javadoc and stack trace are necessary and sufficient to fix those bugs.

I agree.

>> 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.
>
> This is a really tiny subset. Ole's proposal could focus on those few
> cases.

I think the other existing exceptions and the way we handle localization
is fine, and does not really need a refactoring.

The use-cases provided in Ole's proposal look weird and seem again more
suited for something like spring rather than for a low-level library as
CM. There was the case how spring handles exceptions from hibernate, but
we should compare ourselves not with spring but with hibernate, Just my
2cents.

> For the rest, I'm all for being consistently "standard"; meaning that
> in CM we'd write code similar to the following:
>
> /**
>  * Comment on "evaluate".
>  *
>  * @paran n Comment on "n".
>  * @return the result.
>  * @throws IllegalArgumentException if {@code n <= 0}.
>  */
> public double evaluate(int n) {
>   if (n <= 0) {
>     throw new IllegalArgumentException("n <= 0");
>   }
>
>   return compute(n);
> }
>
> KISS.

This is what I had in mind. Thanks for sharing some code snippets.

> And if somewhere else there is
>   throw new IllegalArgumentException("p <= 0");
> I won't consider it duplicate code...

For very common cases there could be utility methods.

Thomas

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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

ole ersoy
In reply to this post by Gilles Sadowski
A few drawbacks to having IAE thrown by CM is that it complicates and blurres things for those designing a handler that catches all CM exceptions.  CM advertising a factory that throws each exception 'type' under globally unique conditions minimizes root cause analysis time and indirection.

This:

   if (n <= 0) {
     throw new IllegalArgumentException("n <= 0");
   }

Misses out on the factory benefit of closing over the condition that checks and throws the exception.  It also makes the explanation for developing and using CM longer...Is it a NOT_STRICTLY_POSITIVE_EXCEPTION or IAE that actually is a NOT_STRICTLY_POSITIVE_EXCEPTION exception?

If I know that it's a NOT_STRICTLY_POSITIVE_EXCEPTION then I'm one step ahead.  Maybe I can simply set the argument to zero and try again, or just throw that step away and continue.

If we standardizes on using Factory.checkNotStrictlyPositiveException(key, n) the client developer can also grab the key and the n value and reconstruct the message.

Also, this:

Factory.checkNotStrictlyPositiveException(key, n)

Is easier, more semantic, less error prone, and faster to write than:

   if (n <= 0) {
     throw new IllegalArgumentException("n <= 0");
   }

And it provides more benefits:
- Parameter name(s)
- Parameter values
- More semantic
- Almost instant path to root cause
- Exception thrown by class (One method call - no unrolling of the cause stack)
- Exception thrown by method (One method call - no unrolling of the cause stack)

Also, if CM modularizes, then the Factory approach standardizes exception generation and handling across the entire ecosystem.

Cheers,
Ole




On 12/23/2015 10:39 AM, Gilles wrote:

> On Wed, 23 Dec 2015 16:26:52 +0100, Thomas Neidhart wrote:
>> 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.
>
> It was consensual.
> What you express below is far more controversial.
>
>> 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.
>
> This is a point I made a long time ago (not "in a far, far away galaxy").
> To which I got the answer that CM must provide a
>  * detailed,
>  * localizable,
>  * textual
> error message.
>
> IMO, for a bug pointed to by an IAE, all the developer has to know is the
> stack trace.
>
> If we want to be "standard", we shouldn't even have to check for null or
> array length on caller's input data as we know that the JVM will do the
> checks and trivially throw standard exceptions on these programming errors!
>
> Javadoc and stack trace are necessary and sufficient to fix those bugs.
>
>> 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.
>
> This is a really tiny subset. Ole's proposal could focus on those few
> cases.
> For the rest, I'm all for being consistently "standard"; meaning that
> in CM we'd write code similar to the following:
>
> /**
>  * Comment on "evaluate".
>  *
>  * @paran n Comment on "n".
>  * @return the result.
>  * @throws IllegalArgumentException if {@code n <= 0}.
>  */
> public double evaluate(int n) {
>   if (n <= 0) {
>     throw new IllegalArgumentException("n <= 0");
>   }
>
>   return compute(n);
> }
>
> KISS.
>
> And if somewhere else there is
>   throw new IllegalArgumentException("p <= 0");
> I won't consider it duplicate code...
>
>
> Gilles
>
>>
>> Thomas
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
> .
>


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

Reply | Threaded
Open this post in threaded view
|

Re: [Math] Exceptions from "JDKRandomGenerator"

Gilles Sadowski
On Wed, 23 Dec 2015 14:08:13 -0600, Ole Ersoy wrote:

> A few drawbacks to having IAE thrown by CM is that it complicates and
> blurres things for those designing a handler that catches all CM
> exceptions.  CM advertising a factory that throws each exception
> 'type' under globally unique conditions minimizes root cause analysis
> time and indirection.
>
> This:
>
>   if (n <= 0) {
>     throw new IllegalArgumentException("n <= 0");
>   }
>
> Misses out on the factory benefit of closing over the condition that
> checks and throws the exception.  It also makes the explanation for
> developing and using CM longer...Is it a
> NOT_STRICTLY_POSITIVE_EXCEPTION or IAE that actually is a
> NOT_STRICTLY_POSITIVE_EXCEPTION exception?
>
> If I know that it's a NOT_STRICTLY_POSITIVE_EXCEPTION then I'm one
> step ahead.  Maybe I can simply set the argument to zero and try
> again, or just throw that step away and continue.

I think that there is a mixing of things:
  1. exception caused by a programming error
  2. exception not caused by a programming error
  3. syntactic sugar (check and throw in one go)

There is the argument that case 1 being a bug, it should not happen and
consequently if an application calls CM with wrong input, it's its
fault,
not CM's, unless the bug is in CM of course. In either case, the stack
trace pinpoints the bug's location. Once the code is fixed, the
exception
will not happen again.
Hence why do anything more than throw the exception to stop the code
where
it is known that there is a problem?
[Arguably, a localized message makes the code harder to debug.]

Using the exception type for control flow is not a good practice. And
assuming that the code can continue after a programming error has been
detected does not look like the best thing to do.

If the error is not a bug, then it makes sense to be able to create a
localized report.

We have to eventually decide whether programming errors _must_ be
standard
or _can_ be non-standard...

> If we standardizes on using
> Factory.checkNotStrictlyPositiveException(key, n) the client
> developer
> can also grab the key and the n value and reconstruct the message.
>
> Also, this:
>
> Factory.checkNotStrictlyPositiveException(key, n)
>
> Is easier, more semantic, less error prone, and faster to write than:
>
>   if (n <= 0) {
>     throw new IllegalArgumentException("n <= 0");
>   }
>
> And it provides more benefits:
> - Parameter name(s)
> - Parameter values
> - More semantic
> - Almost instant path to root cause
> - Exception thrown by class (One method call - no unrolling of the
> cause stack)
> - Exception thrown by method (One method call - no unrolling of the
> cause stack)

That's fine with me, as long as all is done so that localization is
gone from CM's main JAR. ;-)


Regards,
Gilles

> Also, if CM modularizes, then the Factory approach standardizes
> exception generation and handling across the entire ecosystem.
>
> Cheers,
> Ole
>
>>> [...]



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