commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Date Mon, 02 May 2011 08:38:25 GMT
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,
(2) it will not provide the "rich context" feature,
(3) it does not provide the flexibility that an application programmer might
    want for customizing an error message.


Gilles

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


Mime
View raw message