Make ismutable a type-parameter of FunctionMap

[dkarrasch]

This takes a small step towards what I think is wide-spread in the SciML/DiffEq world: making mutability a type-parameter. This allows to solve #193 by multiple dispatch, rather than querying a specific field of all factors/summands. The downside is that previous constructor calls are potentially not fully inferred, see the changed tests. If we advise users, however, to use the FunctionMap constructor explicitly and provide the mutability information, that is most likely statically known anyway, directly, then that shouldn't do much harm. In any case, the current design is such that the "old" behavior still works, it's just deprecated. What I find nice is that for out-of-place FunctionMaps, A*B*C*D*x first folds from the left, so that we first create a CompositeMap until we hit x, from where it folds from the right, without going through 3-arg mul!.

[codecov]

Codecov Report

Base: 99.47% // Head: 99.41% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (8cb8af4) compared to base (d25eb05).
Patch coverage: 99.04% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
- Coverage   99.47%   99.41%   -0.07%     
==========================================
  Files          19       19              
  Lines        1530     1534       +4     
==========================================
+ Hits         1522     1525       +3     
- Misses          8        9       +1     

Impacted Files	Coverage Δ
src/fillmap.jl	100.00% <ø> (ø)
src/functionmap.jl	98.79% <96.96%> (-1.21%)	⬇️
src/LinearMaps.jl	100.00% <100.00%> (ø)
src/blockmap.jl	99.34% <100.00%> (+<0.01%)	⬆️
src/composition.jl	100.00% <100.00%> (ø)
src/linearcombination.jl	100.00% <100.00%> (ø)
src/show.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dkarrasch]

@JeffFessler I suspect you are using FunctionMaps heavily. Can you please try your packages with this PR branch and report back any new issues? I believe nothing should break, only some deprecation warnings should appear, but you never know.

[dkarrasch]

Sorry I messed a few things up while splitting. Seems fixed now.

[JeffFessler]

Ok, I did a test of LinearMapsAA.jl and it passed, but with only one deprecation warning so maybe that package does not actually use so many FunctionMaps. So then I also tested MIRT.jland again it passed with exactly one deprecation warning. Maybe that means that @deprecate warns only once (I could not tell from Julia manual). So I guess all of this bodes well for this PR. I suspect that many of my uses of FMs are spread out in many Literate examples that are not so easy to test, but we'll find out in time...

I have not done much (if any) testing of PR branches of others so I want to describe the approach so that you can confirm if I did it properly. I started Julia 1.8 in an empty directory, then:

] activate .
add LinearMaps#dk/iipfunctionmap
add LinearMapsAA
test LinearMapsAA
add MIRT
test MIRT

So LTGM overall, and I am sure that #193 will bite me too at some point so I am in favor of this direction!

[dkarrasch]

Thank you very much for the detailed report! If you received deprecation warnings (no matter how many), then it grabbed the right branch. I'm glad that tests pass, too. I hope that FunctionMap construction is not done in hot loops, so the deprecation warnings should not yield a performance penalty. I guess this means that it is fine to release this with a minor version bump. For the future, I believe this will allow for some memory optimizations, even though we don't want to dive too deep in there.

[fredrikekre]

A bit of a drive-by comment, but I personally find FunctionMap{T,true}(...) to be less readable compared to FunctionMap{T}(..., ismutating=true). I tried to look at the code, but didn't see where the new parameter is used for dispatch. Does the parameter improve something external or is it just to get rid of ifs internally?

[dkarrasch]

Drive-by comments are always welcome! There is dispatch on IIPFunctionMap vs. OOPFunctionMap in linearcombinations.jl and composed.jl. And yes, it's used to catch cases where all FunctionMaps allocate the result array themselves. I don't have a strong preference, it just reminded me that this has been standard in the DiffEq world for a long time, but it could be solved equally well via branching internally I guess. Any thoughts?

[JeffFessler]

less readable compared to FunctionMap{T}(..., ismutating=true)

I tend to agree. And the kwarg approach has the advantage of being amenable to splatting kwargs...
I prefer that approach for the T= argument too, but I probably am an outlier there.
I am talking about an outer constructor only; I realize the type itself and the inner constructor could have all sorts of parameters like FunctionMap{T, F1, F2, iip}(...)

[dkarrasch]

Hm, another option could be to make LinearMaps.ismutating a "trait" for all LinearMap subtypes. That would allow asking a LinearCombination/CompositeMap whether all its maps are mutating, whereas the current design only catches cases where out-of-place action is due to FunctionMaps. But other, custom map types could have the same, and we would miss that. And we could even ask individual maps and do something with that information.

[dkarrasch]

I realized we already have some kind of the necessary machinery: MulStyle, which so far has only ThreeArg and FiveArg, but which could be extended to TwoArg, meaning A*x. Reflecting in-place vs. out-of-place behaviour reasonably via MulStyle, however, requires that this information is available in the type domain. So how about adding the type parameter iip, but not deprecate the old constructors? The only issue with that is that the iip parameter may not be inferred from the kwarg constructors. If, however, between map construction and extensive usage is a function barrier, that should be no problem. Thoughts?

[JeffFessler]

I think it is fine not to have type inference for the kwarg constructors because (at least in my use cases) construction is done just once in an initialization phase and not in any tight loop. Maintaining the old constructors sounds convenient to me.

[dkarrasch]

These tests are all sharp. They fail on the currently released version!

[dkarrasch]

This is now new-feature-only, nothing breaks (except for the lack of complete type inference for the convenience constructor). Thanks @fredrikekre for retriggering some thought, thanks @JeffFessler for your comments and testing!

[dkarrasch]

BTW, I changed the default values of dest and source in _compositemul! to nothing, and moved the allocation to the function body (if necessary). We don't use these temporary arrays internally, and any external user will take care of the right size and eltype themselves.