make A*I return A #63

dkarrasch · 2019-08-28T08:31:14Z

Related to #57.

coveralls · 2019-08-28T09:26:23Z

Coverage decreased (-6.9%) to 91.546% when pulling a0fc88a on bool_uf into c435822 on master.

coveralls · 2019-08-28T09:26:23Z

Coverage increased (+0.007%) to 98.67% when pulling e8473b8 on bool_uf into 1555f56 on master.

JeffFessler

should
@test LA * I == LA == I * LA
perhaps be
@test LA * I === LA === I * LA
at line 416?
otherwise looks good to me.

JeffFessler · 2019-08-28T13:19:49Z

I don't have enough understanding of type instability to really know the impact, but my hope would be the effect would be small because I imagine the typical use case is something like
B = A * (do_precon ? P : I)
in the preamble of some code, then after that there would be repeated use of B*x in some iterative code. Although I could also imagine something like this
P = do_precon ? make_fancy_P() : I
in the preamble,
followed by repeated use of P * A * x in code, and I'm unsure how the type instability would affect performance there. I'd probably write P * (A * x) for such code anyway which would make it moot...
Anyway, my vote is to merge this PR (perhaps with the test change I suggested) and see what happens :) unless anyone like @Jutho sees a drawback.

dkarrasch · 2019-08-28T16:18:42Z

the typical use case is something like
B = A * (do_precon ? P : I)
in the preamble of some code, then after that there would be repeated use of B*x in some iterative code.

This is exactly the issue that I worry about: at compilation time, the code will not know of which type B is. As I said in the other PR, maybe this is exactly what union splitting is good for, so that "two versions" corresponding to the two possible outcome types are compiled, and eventually everything looks like it was type-stable. The other option is to have a so-called function barrier, so between the generation of B and the B*x there is a function call, at which point the type of B will be known, and a specialized method gets compiled. But maybe the call to mul! or * actually already does serve as a function barrier?

codecov · 2019-08-30T08:59:16Z

Codecov Report

Merging #63 into master will decrease coverage by 6.09%.
The diff coverage is 12.12%.

@@            Coverage Diff            @@
##           master      #63     +/-   ##
=========================================
- Coverage   98.62%   92.52%   -6.1%     
=========================================
  Files           8        8             
  Lines         435      468     +33     
=========================================
+ Hits          429      433      +4     
- Misses          6       35     +29

Impacted Files	Coverage Δ
src/LinearMaps.jl	`100% <ø> (ø)`	⬆️
src/linearcombination.jl	`81.57% <0%> (-18.43%)`	⬇️
src/composition.jl	`94.02% <40%> (-4.36%)`	⬇️
src/uniformscalingmap.jl	`72% <9.52%> (-24.3%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c435822...a0fc88a. Read the comment docs.

codecov · 2019-08-30T08:59:17Z

Codecov Report

Merging #63 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   98.62%   98.83%   +0.21%     
==========================================
  Files           8        8              
  Lines         435      430       -5     
==========================================
- Hits          429      425       -4     
+ Misses          6        5       -1

Impacted Files	Coverage Δ
src/composition.jl	`98.46% <100%> (+0.07%)`	⬆️
src/uniformscalingmap.jl	`95.65% <0%> (-0.65%)`	⬇️
src/blockmap.jl	`99.27% <0%> (+0.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03960a9...e320bee. Read the comment docs.

JeffFessler

i was curious about the multiplication with ZeroMap using
zero(promote_type(eltype(A), eltype(x)), ...)
instead of just the simpler zero(eltype(x),
but i suppose it is consistent with how an actual matrix of zeros would work in multiplication.

Jutho · 2019-09-09T13:51:49Z

src/linearcombination.jl

@@ -37,6 +37,18 @@ function Base.:(+)(A₁::LinearMap, A₂::LinearMap)
    return LinearCombination{T}(tuple(A₁, A₂))
 end
 Base.:(-)(A₁::LinearMap, A₂::LinearMap) = +(A₁, -A₂)
+function Base.:(+)(O::ZeroMap, A::LinearMap)
+    size(O) == size(A) || throw(DimensionMismatch("+"))
+    T = promote_type(eltype(O), eltype(A))


what is the point of computing T if then anyway A is returned?

Good point. This situation here is supposed to occur when the ZeroMap was created out of a CompositeMap that had a ZeroMap factor. So that map does have its own eltype, which probably should not get lost here. So I guess what should be returned is actually LinearMap{T}(A). At least that's what I meant to do originally, but then forgot about it in the next line.

Jutho · 2019-09-09T13:51:58Z

src/linearcombination.jl

+Base.:(+)(A::LinearMap, O::ZeroMap) = +(O, A)
+function Base.:(+)(O₁::ZeroMap, O₂::ZeroMap)
+    size(O₁) == size(O₂) || throw(DimensionMismatch("+"))
+    T = promote_type(eltype(O₁), eltype(O₂))


Jutho · 2019-09-09T13:55:30Z

I am not too fond of the type instability. Another way to resolve this, is to special case the multiplication rule of CompositeMap when one of the maps is a UniformScalingMap. This is likely to be hard though.

Yet another solution is to have a ScaledMap which just encodes a linear map and a scalar factor. ZeroMap would be a special case of that. This functionality could also be added to WrappedMap, though that is maybe throwing together too much.

dkarrasch · 2019-09-12T16:38:14Z

@Jutho Feel free to close this. I agree that we should avoid type instabilities. I think I did what you suggest here

Another way to resolve this, is to special case the multiplication rule of CompositeMap when one of the maps is a UniformScalingMap.

in #69. The first draft is probably a bit hacky, but it works and avoids the issues here. I think it's generally the way to go, so to address algebraic tricks/reductions (which depend on cheap checks on scalars or sizes) in the multiplication functions, rather than on the constructor/type level. I have done something similar now in the Kronecker PR #61.

dkarrasch requested a review from Jutho August 28, 2019 08:48

dkarrasch mentioned this pull request Aug 28, 2019

Make LinearMap * I returns same LinearMap #57

Open

JeffFessler reviewed Aug 28, 2019

View reviewed changes

dkarrasch force-pushed the bool_uf branch from e8473b8 to e320bee Compare August 30, 2019 08:59

dkarrasch added 2 commits September 2, 2019 14:10

make A*I return A

41f12f5

adopt tests to new test suite

c34e39e

dkarrasch force-pushed the bool_uf branch from e320bee to c34e39e Compare September 2, 2019 12:11

JeffFessler approved these changes Sep 3, 2019

View reviewed changes

add ZeroMap type

a0fc88a

JeffFessler reviewed Sep 6, 2019

View reviewed changes

JeffFessler approved these changes Sep 6, 2019

View reviewed changes

Jutho reviewed Sep 9, 2019

View reviewed changes

dkarrasch mentioned this pull request Sep 12, 2019

handle uniform scalings in compositions of maps explicitly #69

Closed

dkarrasch closed this Oct 21, 2019

JeffFessler mentioned this pull request Jul 4, 2020

Add ScaledMap #94

Merged

dkarrasch deleted the bool_uf branch June 27, 2024 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make A*I return A #63

make A*I return A #63

dkarrasch commented Aug 28, 2019 •

edited

Loading

coveralls commented Aug 28, 2019 •

edited

Loading

coveralls commented Aug 28, 2019

JeffFessler left a comment

JeffFessler commented Aug 28, 2019

dkarrasch commented Aug 28, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019

JeffFessler left a comment

Jutho Sep 9, 2019

dkarrasch Sep 9, 2019

Jutho Sep 9, 2019

Jutho commented Sep 9, 2019

dkarrasch commented Sep 12, 2019

make A*I return A #63

make A*I return A #63

Conversation

dkarrasch commented Aug 28, 2019 • edited Loading

coveralls commented Aug 28, 2019 • edited Loading

coveralls commented Aug 28, 2019

JeffFessler left a comment

Choose a reason for hiding this comment

JeffFessler commented Aug 28, 2019

dkarrasch commented Aug 28, 2019

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

codecov bot commented Aug 30, 2019

Codecov Report

JeffFessler left a comment

Choose a reason for hiding this comment

Jutho Sep 9, 2019

Choose a reason for hiding this comment

dkarrasch Sep 9, 2019

Choose a reason for hiding this comment

Jutho Sep 9, 2019

Choose a reason for hiding this comment

Jutho commented Sep 9, 2019

dkarrasch commented Sep 12, 2019

dkarrasch commented Aug 28, 2019 •

edited

Loading

coveralls commented Aug 28, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019 •

edited

Loading