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: [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
