You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Gilles (JIRA)" <ji...@apache.org> on 2011/01/19 14:12:45 UTC
[jira] Created: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
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.
[jira] [Resolved] (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Luc Maisonobe (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Luc Maisonobe resolved MATH-487.
--------------------------------
Resolution: Fixed
Fix Version/s: (was: 2.2)
Fixed in subversion repository as of r1177986.
The checked exception has been replaced by the unchecked one that lies in the exception package.
> 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
> Priority: Minor
> Fix For: 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.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12983737#action_12983737 ]
Phil Steitz commented on MATH-487:
----------------------------------
I am fine with deprecating the checked exception.
I want to make sure, though that we do not lose context information in the exception instances that replace it. The first example above loses information, IMO. The fact that scale is at that point not strictly positive is an implementation artifact, generally useless to the caller. What is actually going on is that the fraction is diverging to infinity (different from diverging to NaN) and *that* is what should be reported.
Honestly, I think that ConvergenceException is not a bad abstraction. Perhaps just make it unchecked? Or define subclasses for more specific convergence failures?
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984700#action_12984700 ]
Gilles commented on MATH-487:
-----------------------------
I know that the different criteria (tolerances, number of iterations, etc.) are somehow linked and that various combinations can lead to a convergence failure. It might be nice to have a class that encompasses all the ways that can lead to such a failure. We don't have one and thus you want to stick with an empty shell {{ConvergenceException}}. This step, instead of improving the situation (e.g. reducing the number of redundant messages) can only make it worse because developers will start use this half-broken place-holder and since there is no state in that class nor design proposal to figure out one, they will just add new messages to convey the exact problem they encounter (and will sometimes create new messages even if an equivalent one already exists... Doesn't that sound familiar?). Furthermore, I'm quite sure that some of those messages will be low-level and others will be high-level (as what is important or implementation detail will vary according to the developer, as shown even in this very short code that is the {{ContinuedFraction}} case); this will become a mess.
The "unfinished" {{ConvergenceException}} is not a compromise because
# It departs from the current work of simplifying the exceptions generation and handling which is reporting the direct cause of the failure, and not try to outguess the caller.
# I'm afraid that the ideal ("one-size-fits-all-combinations-of-convergence-criteria") {{ConvergenceException}} is so complicated that, if not downward impossible to achieve, it will become a sort of framework of its own . At least, it will take a long time before anyone comes up with a working design, leaving ample time to build up the mess I refer to above. That kind of mess is what led to issue MATH-195, and I'm sorry I have to say your compromise paves the way back to square one.
I'm in favour of the "KISS" principle: Let's stick with what is known to be useful (i.e. reporting the problem as CM sees it); let's not base current coding on hope for functionality that nobody knows how to implement.
If the design for high-level exceptions arises some day, it will be relatively easy to wrap the simple, local, low-level exceptions into these new ones.
The current simplification turns out to be quite some work; if we hope that CM will grow, but don't stick to simple rules, we run the risk that it becomes inconsistent and unmanageable.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984614#action_12984614 ]
Luc Maisonobe commented on MATH-487:
------------------------------------
What I expect is that in many cases, convergence exception appear in iterative algorithms where the user does have a criterion setting (number of evaluation, absolute tolerance, relative tolerance, stability check, whatever). We did have different messages for all these cases, it was too many.
In order to reduce the number of different exceptions, we could wrap all these ones into convergence exception as a compromise and given the context the user could work on his settings. Unifying these different errors makes sense because often in numerical analysis, the various criterion are linked together, so you may fix a stability check by changing an absolute tolerance for example.
Of course a single Convergence exception is not perfect but its a compromise solution.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984401#action_12984401 ]
Luc Maisonobe commented on MATH-487:
------------------------------------
I would consider this exception as something not related to MathIllegalNumberException, and as such to inhrits directly from MathRuntimeException.
An empty shell exception is fine to me. It DOES bring information by its type, which can be simply caught.
MathIllegalArgumentException does not have the same meaning because it is, well, not an exception for an illegal argument! Also using a too general exception for too many different things is bad IMHO.
Sorry for another reply, but I am still convinced ConvergenceException is viable and needed. Convergence is an important notion in numerical analysis.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12983931#action_12983931 ]
Gilles commented on MATH-487:
-----------------------------
What bothers me is that I don't see any added value to such seemingly "high-level" exceptions. Primarily because the first rule is that the exception should be thrown as close as possible to the source of the problem. And I think that the exception should describe the local problem, and not try to convey what a local failure means for the caller. In this case, we have
# The callee says: "Hey, this quantity is infinite!"
# The caller figures out: "The ContinuedFraction has a divergence problem."
I think that, CM being a low-level component, it is fine to throw low-level exceptions. The callee encounters a problem and signals it faithfully to the caller; the caller is then free to pass on the information and change (or wrap) it into a higher level type such as "ConvergenceException". It can do so because the CM code must have a comment stating:
{noformat}
/**
* @throws NotFiniteNumberException when the continued fraction fails to converge.
*/
{noformat}
In the above Javadoc excerpt, the type provides a description of the (local) problem and the comment describes the cause of the problem. They are complementary while, if using a "ConvergenceException" type, the comment becomes redundant.
Moreover, I think that mixing low-level ("MaxCountExceededException") and high-level abstractions ("ConvergenceException") does not make for particularly clear code (i.e. when to use which level?). Again, it's not that I don't like the idea of high-level exceptions, it's just that I don't see how to make them coexist with low-level ones.
Creating an unchecked "ConvergenceException" and is certainly very easy, but I don't see it as so clear-cut what the best solution is.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984707#action_12984707 ]
Phil Steitz commented on MATH-487:
----------------------------------
Can we please take this general discussion to the mailing list?
If is easier to respond to the various points made there and the scope of this - important - discussion is well beyond this issue.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12983765#action_12983765 ]
Gilles commented on MATH-487:
-----------------------------
{quote}
[...] The first example above loses information, IMO. [....]
{quote}
Yes; that's why I propose to implement the last excerpt.
Note that the exception ideally has to report _what_ is going wrong; in the first case, the problem is the value of the {{scale}} variable.
However I can agree that it is an implementation detail, so that at that point, instead of duplicating the "throw" statement we could just do a "break", and only report the "infiniteness" failure. Is that OK?
{quote}
[...] I think that ConvergenceException is not a bad abstraction. [...]
{quote}
I agree. However, I can also argue that it is an implementation detail; as you said, the important thing
{quote}
[...] is that the fraction is diverging to infinity [...]
{quote}
and I think that this problem is better represented by the {{NotFiniteNumberException}} abstraction (which also specifies whether the problem is "infinity" or "NaN").
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12983858#action_12983858 ]
Luc Maisonobe commented on MATH-487:
------------------------------------
I fully agree with Phil.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984154#action_12984154 ]
Phil Steitz commented on MATH-487:
----------------------------------
Another way to think about this is what abstraction makes sense at the API boundary where the exception is being propagated. In this case, it is obvious to me that "convergence exception" is much better than "some number became infinite." A well-designed exception hierarchy enables the right abstraction to be expressed in the different exception use cases, which to me means that the exception conveys information to the caller that makes sense in the context of the public API (i.e., without having to look at the code or know the details of the implementation). When a continued fraction (or quadrature method, or rootfinder, or....) fails to converge, conveying that directly in the exception is appropriate, IMO. We can certainly talk about how this abstraction may fit into a more fully articulated hierarchy, but I am -1 on just dropping it at this point.
While we all agree that we need a better-designed hierarchy, we are struggling with how that hierarchy should be structured and how it should be connected to the concepts in the Commons Math API. I think it is best at this point to close this issue, but reopen the discussion on the mailing list on how best to structure our exceptions hierarchy. I think we first need to agree on some principles. If what I have stated above about connection to the context of the public API, for example, is not acceptable, we need to reach consensus on an alternative that we can all support. I apologize for "more complaining than coding" in this area up to now. I will fix that.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Gilles (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984502#action_12984502 ]
Gilles commented on MATH-487:
-----------------------------
I'm not an expert in numerical analysis, and I certainly don't want to imply that the concept of convergence is not important. I still can say that in the current {{ConvergenceException}}, the link to this concept is too vague. You want to convey a high-level notion (convergence problem when evaluating a continued fraction at "x") but you don't say what happened exactly. Which I think doesn't make sense at the CM level; in all other cases where exceptions are thrown, we explain what, exactly, went wrong (e.g. how the precondition was violated). Here, should we be content to say that something called "convergence" went wrong?
If the problem in the {{evaluate}} method is a numerical one, then maybe that the appropriate exception type is more akin to {{MathArithmeticException}}. Moreover, I think that if the potential infinities and NaNs arise from numerical problems, it should not be considered as an implementation detail but reported as such.
An empty shell is not fine; it is an indication that something is missing.
{quote}
[...] using a too general exception for too many different things is bad [...]
{quote}
I totally agree, and that's why I'm saying that {{ConvergenceException}} without any state is useless: What do you expect to be able to do by catching specifically a stateless {{ConvergenceException}} that you wouldn't be able to do by catching, say, a {{MathRuntimeException}}?
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Luc Maisonobe (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12984081#action_12984081 ]
Luc Maisonobe commented on MATH-487:
------------------------------------
ConvergenceException not a so high abstraction level, so it seems to fit with the library level.
Also redundancy appears only to someone who has accesss to the code and as stated many times, not all users are developers, the low level exception may slip upward, ... It's the same story again.
Another point is that this exception may be convenient to users that implement their own algorithms too.
> 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.
[jira] Commented: (MATH-487) Deprecate "ConvergenceException" in
MATH_2_X and remove it in trunk
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12983831#action_12983831 ]
Phil Steitz commented on MATH-487:
----------------------------------
In the case of a continued fraction, I don't see convergence failure as an implementation detail - it is an abstraction suitable to the domain. NotFiniteNumberException does not convey the same or even similar semantics.
I don't want to argue about this, especially given my lack of energy to put into the refactoring; but I think it is a good idea to think about some high level abstractions such as convergence failure in refactoring the exception hierarchy along with low-level constructs such as NumberTooSmall, etc. This is a good example where without the high level abstraction, what surfaces from the API is just that some number became infinite - not the more domain-specific information that the iterates diverged. To retain this context information, we need to rely on the exception message. So in this case, we are actually going in the opposite direction of what we have been trying to do, which is to rely less on the messages and more on the exception hierarchy itself to convey context info.
> 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.