commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gilles (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MATH-487) Deprecate "ConvergenceException" in MATH_2_X and remove it in trunk
Date Thu, 20 Jan 2011 15:27:45 GMT

    [ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984223#action_12984223
] 

Gilles commented on MATH-487:
-----------------------------

To be sure, it is always better in principle to aim for high-level abstractions.
It's fine to discuss later what you'd propose as a design. At least, we won't have the same
amount of refactoring work once all the exceptions are unchecked (and that's not a small advantage,
IMO).

Last point, for the record, before leaving the "ContinuedFraction" issue for now.
How does {{ConvergenceException}}  connect with what exists in the {{exception}} package?
I mean: Is it supposed to be a kind of {{MathIllegalNumberException}} or is it something completely
different (that inherits directly from {{MathRuntimeException}})?

If it is the former, then we would say that it is a mistake to call {{evaluate}} with some
(illegal argument) {{x}}. This is consistent with the other exceptions of this kind: {{x}}
is the "wrong" value (to be returned with the base class's {{getArgument}} method).
But in that case, {{ConvergenceException}} is indeed low-level (as Luc said) and it can't
be reused in a context where you don't have such an {{x}} parameter (e.g. convergence failure
in solvers and optimizers). Then the name "ConvergenceException" is certainly too general.
{{ContinuedFractionConvergenceException}} maybe? Or maybe that that one is too specific, and
will precludes its potential reuse in similar contexts but unrelated to the concept of continued
fraction...

If it is the latter, then what would be the instance variable(s) of {{ConvergenceException}}?
If there are none, and the only "added value" is the message enum, then why bother with yet
another "empty shell" class?

In summary, we could have:
{noformat}
  throw new MathIllegalArgumentException(LocalizedFormats.INFINITY_DIVERGENCE,
                                         LocalizedFormats.CONTINUED_FRACTION,
                                         x);
{noformat}
What I mean is that, in some rare cases such as this one, we could use the more general {{MathIllegalArgumentException}}
which sufficiently flexible to convey all the existing information:
* kind of divergence (NaN vs Infinity)
* context (continued fraction)

(please note the factorization of the enum into a "general" and "specific" part).

I could also change the accessibility of the constructors of {{MathIllegalNumberException}}
(from {{protected}} to {{public}}) and use that class instead of {{MathIllegalArgumentException}}.
I think that this is the most sensible intermediate solution (until there is a solid design
that can deal uniformly with all cases).

Sorry for another long-winding post, but I hope I've shown that this {{ConvergenceException}}
class is not viable in the long term.


> Deprecate "ConvergenceException" in MATH_2_X and remove it in trunk
> -------------------------------------------------------------------
>
>                 Key: MATH-487
>                 URL: https://issues.apache.org/jira/browse/MATH-487
>             Project: Commons Math
>          Issue Type: Improvement
>            Reporter: Gilles
>            Assignee: Gilles
>            Priority: Minor
>             Fix For: 2.2, 3.0
>
>
> The checked "ConvergenceException" should be deprecated.
> An example usage is in class {{ContinuedFraction}} (package {{util}}), at line 153:
> {noformat}
>   if (scale <= 0) {  // Can't scale
>     throw new ConvergenceException(LocalizedFormats.CONTINUED_FRACTION_INFINITY_DIVERGENCE,
x);
>   }
> {noformat}
> I think that it should be replaced by a more specific (and unchecked) exception that
reflects the exact low-level problem:
> {noformat}
>   if (scale <= 0) {  // Can't scale
>     throw new NotStrictlypositiveException(scale);
>   }
> {noformat}
> A few lines below that, there is:
> {noformat}
>   if (infinite) {
>     // Scaling failed
>     throw new ConvergenceException(LocalizedFormats.CONTINUED_FRACTION_INFINITY_DIVERGENCE,
x);
>   }
> {noformat}
> So it seems that it is not necessary to throw an exception at the place where the test
on "scale" fails; instead we could have:
> {noformat}
>   infinite = true;
>   if (scale <= 0) {  // Can't scale
>     break;
>   }
> {noformat}
> and let the check on "infinite" throw the exception:
> {noformat}
>   if (infinite) {
>     // Scaling failed
>     throw new NotFiniteNumberException(LocalizedFormats.CONTINUED_FRACTION_DIVERGENCE,
>                          Double.POSITIVE_INFINITY, x);
>   }
> {noformat}
> As shown in the above excerpt, we could also replace two {{enum}}:
> * CONTINUED_FRACTION_INFINITY_DIVERGENCE
> * CONTINUED_FRACTION_NAN_DIVERGENCE
> with a single one:
> * CONTINUED_FRACTION_DIVERGENCE
> because the other bit of information (infinity vs NaN) is already given by the first
parameter of the message.
> What do you think of these changes?

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message