linear sum input order #660
Conversation
maggul
requested review from
balos1,
gardner48,
cswoodward and
drreynolds
as code owners
CHANGELOG.md
Outdated
| @@ -6,6 +6,10 @@ | |||
|
|
|||
| ### New Features and Enhancements | |||
|
|
|||
| The `N_VLinearSum` and `N_VLinearSumVectorArray` operations now assume that the output | |||
| can be used as input only in the first component, ensuring compatibility with external | |||
There was a problem hiding this comment.
This seems sort of like we are breaking the API, which would require a new major version. The reason I say "sort of" is because the existing vectors would still work since our change is in the assumption not the implementations. However, the change in the assumption now instructs us to create new vectors that would possibly not work with old code.
There was a problem hiding this comment.
I disagree, since nothing about this PR breaks any existing/working code.
The only thing that would "break" is if a user creates an N_Vector that works with SUNDIALS v7.2.1, and then tries to use that same vector with an older version of SUNDIALS. If anything, the documentation update could add a .. versionchanged comment to note that previous SUNDIALS versions required that all vectors support the case where either input could also be the output.
There was a problem hiding this comment.
I disagree, since nothing about this PR breaks any existing/working code.
That why I said it was "sort of" a breaking change. It changes the contract of the API, therefore pedantically speaking it is a breaking change to the API. However, in practice, its impact is more like a "new feature".
Mustafa was not referring to external libraries with NVector implementations that are included in SUNDIALS; this comment is relevant to user-supplied NVector interfaces to external libraries (in this case, Gkeyll). I do not see any need to add comments to any NVector implementations in the SUNDIALS repository, only to make the assumptions on the SUNDIALS end clear. |
There was a problem hiding this comment.
Here are a few suggestions to resolve @balos1's concern about this PR being a kind-of breaking change.
An
N_VLinearSuminput/output issue arises whenN_VLinearCombinationis not provided by the user or whenN_VLinearSumfollows a different path than the one followed by SUNDIALS. The output is allowed as an input only in the first component for external library compatibility.To that end, we have changed the order of inputs if the output vector is in the second component.
N_VLinearSum(a, x, b, z, z) →N_VLinearSum(b, z, a, x, z)