[
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.
