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 EmbeddedMap #174

Merged
merged 4 commits into from
May 19, 2022
Merged

Add EmbeddedMap #174

merged 4 commits into from
May 19, 2022

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Apr 4, 2022

This is a rebase of #65. Closes #65.

Dropping the requirement of monotonically increasing indices would allow to define "lazy" reversed maps, though negative strides in BLAS.gemv! is allowed starting from v1.8 only.

I wonder if IndexMap is the best name for it, though. "Index" is fairly generic, so we may want to spend some brain power to come up with a better name.

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #174 (3835309) into master (df9ccd3) will decrease coverage by 1.24%.
The diff coverage is 65.11%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   99.65%   98.40%   -1.25%     
==========================================
  Files          14       15       +1     
  Lines        1151     1193      +42     
==========================================
+ Hits         1147     1174      +27     
- Misses          4       19      +15     
Impacted Files Coverage Δ
src/embeddedmap.jl 62.50% <62.50%> (ø)
src/LinearMaps.jl 100.00% <100.00%> (ø)

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 df9ccd3...3835309. Read the comment docs.

@dkarrasch dkarrasch force-pushed the dk/indexmap branch 2 times, most recently from a589c1a to 9e99938 Compare April 5, 2022 13:25
@dkarrasch
Copy link
Member Author

If I'm not mistaken, then this PR coincides greatly with LiftedMaps.jl, so @krcools may want to take a look and make suggestions.

@dkarrasch
Copy link
Member Author

Another thing: if one ignores the monotonic indices test, then this allows for lazy reverseal of matrices/maps! For matrices, that requires negative strides in BLAS, which is available only from v1.8, but still, another nice lazy feature.

@dkarrasch
Copy link
Member Author

@krcools What do you think: I'm finding the name IndexMap too generic, so I was searching for other names, ideally with some precedence. Then I found your LiftedMaps, that has essentially the same functionality, and I assume some precedence regarding the name. What are your plans with your package? Do you think there is a possibility that we name the maps defined here as LinearMaps.LiftedMaps? Or shall we keep things independent and search for another name? It would be good to finish this and release it soon as v3.7, together with some of the other open PRs.

@krcools
Copy link
Contributor

krcools commented May 18, 2022

I am using LiftedMaps in my own registered projects and will keep doing so. Functionality is indeed similar; a noteworthy difference is that LiftedMaps tries to be compatible with non-traditional axes (such as the blocked axes from BlockArrays).

I guess if the name is not exported both implementations can happily coexist.

Alternative names? Perhaps ExtendedMaps, EmbeddedMaps, SingleBlockMaps?

@dkarrasch
Copy link
Member Author

Thanks for your feedback @krcools! Yes, we have avoided the dependency on BlockArrays.jl when we added the BlockMap functionality. I think I'll go with EmbeddedMap, that comes close to what I suggested in #134, which was closed in favor of the predecessor of this PR. Any name requires explanation, but EmbeddedMap comes with a little bit more context than just IndexMap.

@dkarrasch dkarrasch changed the title Add IndexMap Add EmbeddedMap May 18, 2022
@dkarrasch dkarrasch merged commit 98843dd into master May 19, 2022
@dkarrasch dkarrasch deleted the dk/indexmap branch May 19, 2022 07:13
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.

2 participants