Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IndexableMap #145

Closed
wants to merge 2 commits into from
Closed

Add IndexableMap #145

wants to merge 2 commits into from

Conversation

dkarrasch
Copy link
Member

This adds a type for indexable LinearMaps. Construction is via a keyword argument getind, naming can be discussed, of course.

Closes #38.

@dkarrasch dkarrasch mentioned this pull request Apr 14, 2021
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #145 (3f36f3d) into master (032832e) will decrease coverage by 3.16%.
The diff coverage is 36.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   99.80%   96.64%   -3.17%     
==========================================
  Files          14       15       +1     
  Lines        1042     1072      +30     
==========================================
- Hits         1040     1036       -4     
- Misses          2       36      +34     
Impacted Files Coverage Δ
src/composition.jl 97.46% <0.00%> (-2.54%) ⬇️
src/indexablemap.jl 0.00% <0.00%> (ø)
src/scaledmap.jl 92.59% <0.00%> (-7.41%) ⬇️
src/LinearMaps.jl 96.59% <83.33%> (-1.09%) ⬇️
src/conversion.jl 100.00% <100.00%> (ø)
src/kronecker.jl 98.42% <100.00%> (-1.58%) ⬇️
src/uniformscalingmap.jl 100.00% <100.00%> (ø)
src/blockmap.jl 99.09% <0.00%> (-0.91%) ⬇️
... and 2 more

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 032832e...3f36f3d. Read the comment docs.

@@ -293,7 +294,9 @@ For the function-based constructor, there is one more keyword argument:
The default value is guessed by looking at the number of arguments of the first
occurrence of `f` in the method table.
"""
LinearMap(A::MapOrVecOrMat; kwargs...) = WrappedMap(A; kwargs...)
LinearMap(A::MapOrVecOrMat; getind=nothing, kwargs...) = _LinearMap(getind, A; kwargs...)
Copy link
Member

@JeffFessler JeffFessler Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LinearMap(A::MapOrVecOrMat; getind=nothing, kwargs...) = _LinearMap(getind, A; kwargs...)
LinearMap(A::MapOrVecOrMat; getind=Base.getindex, kwargs...) = _LinearMap(getind, A; kwargs...)

For a WrappedMap could the default getind simply be Base.getindex because presumably that is the expected behavior for a matrix or even for any AbstractArray that already has a getindex method.

Base.transpose(A::IndexableMap) = IndexableMap(transpose(A.lmap), (i,j) -> transpose(A.getind(j,i)))
# rewrapping preserves indexability but redefines, e.g., symmetry properties
LinearMap(A::IndexableMap; getind=nothing, kwargs...) = IndexableMap(LinearMap(A.lmap; kwargs...), getind)
# addition/subtraction/scalar multiplication preserve indexability
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!
I wish I had a suggestion for how to handle a CompositeMap...

@Jutho
Copy link
Collaborator

Jutho commented Apr 14, 2021

I won't have time to review this before next week, but I just thought of another possible approach. We could support a general getindex keyword argument in FunctionMap, the default value of which is a function that evaluates the multiplication with a unit vector with a single entry one, but where this default function refuses to work depending on the value of a global switch, similar to how CUDA.jl has allowscalar . The user then has two options to have support for getindex, either by providing a getindex method upon construction of the LinearMap, or by setting allowscalar(true), knowing that this might lead to slow/suboptimal code. Not sure what is needed to set this up, but this might be useful and does not require the introduction of new LinearMap subtypes.

# UniformScalingMap(λ, M) : error("UniformScalingMap needs to be square"))
# UniformScalingMap(λ::T, sz::Dims{2}) where {T} =
# (sz[1] == sz[2] ?
# UniformScalingMap(λ, sz[1]) : error("UniformScalingMap needs to be square"))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Base.getindex(A::UniformScalingMap, args...) = A.λ * A.lmap.getind(args...)

Seems like something like this should be the natural default for this type.

@JeffFessler
Copy link
Member

I was going to suggest something similar to what @Jutho said, i.e., some way of letting users "opt in" to the natural default. It could be a switch or an exported convenience routine that could be passed back in like getind=getind_unitvector_default. The switch seems easier...

I put in a couple other suggestions.

@dkarrasch dkarrasch deleted the dk/indexable_map branch June 24, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why no getindex() ?
3 participants