commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Mon, 02 May 2011 15:04:52 GMT
On 5/2/11 1:38 AM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 03:18:20PM -0700, Phil Steitz wrote:
>> On 5/1/11 2:29 PM, Gilles Sadowski wrote:
>>> On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
>>>> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
>>>>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>>>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>>>>> Converting some of my code to use trunk, I discovered that
the
>>>>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>>>>> when parameters are invalid.
>>>>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>>>>> The advantage being commonly agreed on was to offer the "map"
functionality
>>>>>>> for adding messages and context information.
>>>>>> I guess I misunderstood and after really seeing the consequences
in
>>>>>> my own code, I am going to have to ask that we reopen that
>>>>>> discussion - i.e., I would like us to throw IAE and other standard
>>>>>> exceptions where appropriate, as in this case, as we have up to and
>>>>>> through 2.2.  I know I said before that I did not see this as worth
>>>>>> arguing about, but I really think this change is a bad API design
>>>>>> decision that will cause needless hassle and surprising RTEs for
>>>>>> users upgrading to the new version.
>>>>> I'm astonished, and for the time being, will refrain from other comments.
>>>>>
>>>>>>>> The javadoc asserts that
>>>>>>>> MathIllegalArgumentException will be thrown in these cases,
but that
>>>>>>>> is not correct,
>>>>>>> I don't understand; the code for "checkBinomial" can throw
>>>>>>> "NumberIsTooLargeException" or "NotPositiveException" and they
are
>>>>>>> subclasses of "MathIllegalArgumentException".
>>>>>>>
>>>>>> Sorry. my mistake.
>>>>>>>> since what is actually thrown now can differ
>>>>>>>> depending on the parameter problem
>>>>>>> That's a feature, naturally: Different problem, different exception.
>>>>>>>
>>>>>> That's where I disagree.  I see zero value added and in fact
>>>>>> confusing complexity introduced by these exceptions.  When you ask
>>>>>> for B(n,k) where k is not less than or equal to n, a standard IAE
>>>>>> with a message that says precisely that (which the current message
>>>>>> does) is clear and *better* that a "NumberIsTooLargeException". 
>>>>>> What number?  I guess it must be k?  To figure it out you have to
>>>>>> look at the exception message, which is *exactly the same message*
>>>>>> that the old code reported.  If we really think we need to
>>>>>> specialize and report different exceptions for every precondition
>>>>>> violation (which I do not agree with), then these exceptions should
>>>>>> be meaningful in the context of the API that is using them.  So
>>>>>> here, for example, we would have to throw something like
>>>>>> "CombinationSizeTooLargeForSetException." 
>>>>> Then, we do _not_ disagree _now_. From the start, I stated that a consistent
>>>>> design would be to define a specific exception for each specific that
must
>>>>> be reported, especially if it must contain complex functionality like
>>>>> localization.
>>>>> IIRC, either you or Luc (or both) did not want a large number of exceptions.
>>>>> To keep the number down, we reuse less specific exception types (like
>>>>> "NumberIsTooLargeException" in several contexts) and rely on the message(s)
>>>>> for context information. Nothing lost from the previous situation (when
one
>>>>> *had* to rely solely on the message)!
>>>> But in this case, my opinion is that IAE is just fine - there is
>>>> nothing more "specific" to communicate unless you want to define
>>>> something meaningful in the context of the API. "NumberIsTooLarge"
>>>> has no value here.
>>> I don't agree. This is the concept that describes the problem: indeed, the
>>> precondition is that "k" must be smaller than "n".
>>> This has the same value as the "out of range" concept where you give the
>>> value of some parameter and the values of the bounds.
>>>
>> No, it is actually a different concept.  In mathematical terms, it
>> is a binary relation that is failing here:  k < n.  What
>> "NumberIsTooLarge" (or "NumberIsTooSmall" which could also be
>> applied here, to n instead of k) conveys is unary.
> I repeat that I'd be fine with adding an exception that would retain
> the binary relationship. If you think it is overkill, fine too. But that
> does not make "NumberIsTooLargeException" useless; it is only not precise
> enough in this case to fully describe the mathematical situation. From a
> programming viewpoint, it is IMO a good compromise: It says something about
> how the precondition was tested:
> ---
>   if (k > n) {
>     throw new NumberIsTooLargeException(k, n);
>   }
> ---
>
>>>>  As I said, if we feel we need to make this
>>>> particular exception due to precondition violation more precise, we
>>>> would need to define an exception that refers to subset relation
>>>> size or somesuch, which I don't see as necessary or valuable.
>>> In principle, I've nothing against devising a deeper hierarchy that
>>> would make the type of the exception refer to the exact problem. However,
>>> there would indeed be not much practical value added if all use cases are
>>> about letting the exception bubble upwards until it aborts the program (at
>>> which point a human will read the error mesage).
>> Sounds like you are arguing here to just leave it as IAE, which is
>> fine by me.
> No, see below.
>
>>>>> To answer your question above: No, you don't have to "guess" which number
is
>>>>> too large; there is an accessor "getArgument()" that returns the number
that
>>>>> triggered the exception and another "getMax()" that informs you about
the
>>>>> maximum allowed number.
>>>> No, all that is reported is the *value*.
>>> Well, of course: CM is a numerical library, not a symbolic one.
>>>
>>>> To make this actually
>>>> work, you would have to do something like store and report the
>>>> formal parameter name.  I don't see the point in this in general
>>> Me neither.
>>>
>> Then I would claim "NumberIsTooLarge" and "NumberIsTooSmall" provide
>> no value.  You need to look at the exception message, which
>> thankfully we have preserved in this case, to understand what the
>> actual problem is.  The abstraction itself is worthless at least in
>> this case, IMO.
>>
>>>> and
>>>> certainly not in this case, as what is problematic is the order
>>>> relation among the two parameters - not one is "too small" or the
>>>> other is "too large" but that they violate the stated preconditions
>>>> of the method.
>>> I really don't understand what bothers you!
>>> The precondition
>>>   k <= n
>>> means that if k > n, then the *value* of k is too large.
>>> You can elaborate on the context if you wish, but that does not change the
>>> basic problem.
>> You are missing the point, stated above.  The problem is that we
>> have a precondition that is a *relation* between the two parameters
>> and that precondition has been violated.  Throwing in a
>> context-unaware unary predicate that says "one of the parameters is
>> too large" adds nothing to precise the problem.
> No, in my view, you are missing my many points.
> I understand your point about binary relation but, I repeat, CM is not aimed
> to be a faithful representation of all mathematical concepts; it is aimed at
> solving problems numerically. And to aid debugging when something goes wrong,
> Java provides a mechanism called "exceptions".
> Concerning the abstraction "number is too large", I find it useful just for
> that. Exactly as "out of range" gives a value plus two other values, claiming
> that the first one is not in the range defined by the other two.
>
> If you want that CM throws subtypes of IAE when a precondition fails, that's
> fine with me. It's not fine with me to throw a plain IAE because
> (1) that is not what we agreed on earlier to mitigate the ugliness (in my
>     view) of having localization an integral part of CM,
This has nothing to do with messages.  The "NumberIsTooLarge" thing
still carries *exactly the same message* which is what is really
relevant to the caller.
> (2) it will not provide the "rich context" feature,
Look carefully at how the code now appears *from the standpoint of
the caller* reading the javadoc.  There is nothing useful added as
the exception conveys nothing meaningful beyond IAE.
> (3) it does not provide the flexibility that an application programmer might
>     want for customizing an error message.
Don't follow this.

In any case, I give up.  I can see we are getting nowhere here.

Phil
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message