Math.isClose Promotion Support

I was curious why Math.isClose does not support element-wise operation on arrays? Looking at the source it explicitly rejects arrays.

My only thought was because there was ambiguity on whether or not the function should return a single bool or an array of bool for that case. My vote would be the latter, and if the user wanted a single bool they could apply a reduction.

1 Like

Hi Jared —

I don't have an authoritative answer to this, but I have a good guess that I'll pass along in the interest of getting you a quick response. When @lydia (who did the implementation) is back at work, she may be able to give a sense as to whether my guess is accurate or not and provide more details.

My guess is that the lack of support for isClose() between two arrays, or between an array and a scalar, is more a reflection of the evolution of the implementation than a deeply intentional design choice. And with a quick look at some of the issues around the time we were stabilizing this for Chapel 2.0, I'm not seeing any indication that we considered and rejected support for array-based cases.

Looking at the history of the implementation, it started out in commit ad4e07ffc513 as a generic function on the arguments being compared (x and y), as it is today. When an argument is fully generic like that, it means passing in an array in will treat x like an array rather than considering it an opportunity for promoting the argument. Meanwhile, the implementation of the function used (and uses) short-circuiting operators || to compute its result, and these are not (currently) supported on arrays, so passing an array in during those early commits would result in an error about || not being promotable, which wasn't particularly good as a user-facing error message. So I think the current explicit check against arrays was designed to generate a better error for users for a case that we'd put not effort into supporting.

It could also be that we did discuss adding array support, didn't want to take the time to design or implement it at that point, and so put it off, figuring we could always add it later since it's not a breaking change.

I think it would be very reasonable to file a feature request issue requesting to extend it to support arrays if you'd like to do that. The following represents a very quick sketch that I believe is correct (but may benefit from more work to generate better error messages? and we'd probably want to hoist the repeated default argument values into module-level [config?] params to avoid the chance that they diverge over time):

  proc isClose(X:[], Y:[], relTol = 1e-5, absTol = 0.0) {
    return [(x,y) in zip(X, Y)] isClose(x, y, relTol, absTol);
  }

  proc isClose(X:[], y, relTol = 1e-5, absTol = 0.0) {
    return [x in X] isClose(x, y, relTol, absTol);
  }

  proc isClose(x, Y:[], relTol = 1e-5, absTol = 0.0) {
    return [y in Y] isClose(x, y, relTol, absTol);
  }

Hope this helps,
-Brad

1 Like

If I had an array, I would want to look more deeply at the statistics of closeness throughout the array rather than just use the logic provided by isClose().

Providing a routine for an array using the current logic of isClose() encourages a decision process within a user program that I would argue runs a high risk of oversimplifying the convergence problem and may not give the user a robust result for a given problem. Then again, I might be paranoid. I am often looking for very high precision and I am looking at problems that are of interest to my colleagues and clients which by definition is a subset of all possible problems.

Even where I wanted the logic behind isClose(), from the perspective of code review, I would want the logic of isClose() exposed rather than hidden.

My 2c.

1 Like

Thank you Brad, that looks reasonable to me. I am less interested in this feature though than an analogous one for UnitTest, which would behave slightly differently ( [Feature Request]: assertClose() method for UnitTest · Issue #28099 · chapel-lang/chapel · GitHub ). Though maybe something like this could be generally useful for data analysis or sanity checking in a process.

I strongly agree with this sentiment. Array-of-bool is what you get if you do == on two arrays. isClose should be consistent with that behavior.

1 Like

Hi Jared,

I don't have much to add here, Brad covered the history and reasoning
well. There are definitely design questions about what the function
should return when dealing with arrays, like you and others allude to,
and I think it would be interesting to discuss them, but am happy for us
to focus on what's most relevant to you right now if you don't feel like
opening a second issue :slight_smile:

Thanks,
Lydia

1 Like

Thank you Lydia,

I could open a 2nd issue on this and discussion can continue there. I have time I could submit a pull request too based on Brad's implementation sketch above.

Best,

Jared