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: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
>>
>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
