SciML / SciMLOperators.jl

SciMLOperators.jl: Matrix-Free Operators for the SciML Scientific Machine Learning Common Interface in Julia
https://docs.sciml.ai/SciMLOperators/stable
MIT License
42 stars 9 forks source link

Refactor SciMLOps after #200 #201

Closed vpuri3 closed 1 year ago

vpuri3 commented 1 year ago

the changes in #200 introduced batch kwarg to functionoperator. The API was that with batch = true, the last dimension of u will be the batch dim. With batch = false, the ND input array will be treated as one "column vector". This caused a lot of size mismatch errors since the rest of SciMLOperators takes the second dimension to be the batch dimension as that is what makes sense for matrices. And cases with ND arrays were handled with reshaping to 2D arrays. The principle issue became that we had different measures of length: length(u) for FunctionOperator with batch = false, size(u)[1:end] |> prod for FunctionOP with batch = true, and size(u, 2) for the rest. For an ND array and an arbitrary wrapper, it would be hard to know which measure to use. An increased reliance on reshape also causes confusion that may lead to unsafe behavior. Eg. applying matrices to arrays of wrong sizes should result in errors but could be done with reshape.

With this PR, I am limiting FunctionOperator w. batch = true to only accept 2D arrays where the second dimension is the batch dimension. This is consistent with the rest of the package. FunctionOperator w. batch = false is still able to work with ND arrays as before. Further, I am removing operator evaluation and caching methods for ND arrays to avoid confusion. The only operator that is able to work with ND arrays is FunctionOperator w. batch = false. The rest of the operators work only with AbstractVecOrMats where (optionally) batches are stacked in the 2nd dimension.

new batch API for FunctionOperator

batch - Boolean indicating if the input/output arrays comprise of batched column-vectors stacked in a matrix. If true, the input/output arrays must be AbstractVecOrMats, and the length of the second dimension (the batch dimension) must be the same. The batch dimension is not involved in size computation. For example, with batch = true, and size(output), size(input) = (M, K), (N, K), the FunctionOperator size is set to (M, N). If batch = false, which is the default, the input/output arrays may of any size so long as ndims(input) == ndims(output), and the size of FunctionOperator is set to (length(input), length(output)).

This is a major update. Please tag 0.3.0 after merging.

codecov[bot] commented 1 year ago

Codecov Report

Merging #201 (5eaf445) into master (bc5a1fe) will increase coverage by 3.74%. The diff coverage is 67.50%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   65.46%   69.20%   +3.74%     
==========================================
  Files          11       10       -1     
  Lines        1523     1523              
==========================================
+ Hits          997     1054      +57     
+ Misses        526      469      -57     
Impacted Files Coverage Δ
src/SciMLOperators.jl 100.00% <ø> (ø)
src/interface.jl 44.44% <21.73%> (-12.70%) :arrow_down:
src/func.jl 79.83% <66.66%> (+1.87%) :arrow_up:
src/scalar.jl 54.60% <87.50%> (+4.60%) :arrow_up:
src/basic.jl 71.42% <100.00%> (+4.84%) :arrow_up:
src/matrix.jl 67.02% <100.00%> (+1.94%) :arrow_up:
src/tensor.jl 78.89% <100.00%> (+0.84%) :arrow_up:
src/utils.jl 100.00% <100.00%> (+26.66%) :arrow_up:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

vpuri3 commented 1 year ago

looks like rm'ing the redundant functionop constructor didn't break anything. good

vpuri3 commented 1 year ago

@ChrisRackauckas

vpuri3 commented 1 year ago

@ChrisRackauckas can you take a second look sometime soon? this is holding some stuff up. thanks