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 range indexing to UniformScaling #24359

Merged
merged 12 commits into from
Jun 6, 2020
Merged

Conversation

dalum
Copy link
Contributor

@dalum dalum commented Oct 27, 2017

Ref: JuliaLang/LinearAlgebra.jl#454. I thought this might be a usable alternative to eye and having callable UniformScaling.

@stevengj
Copy link
Member

stevengj commented Oct 27, 2017

It's kind of unsatisfying to have this type-unstable getindex. I'm not sure I would encourage people to use an API like this.

Maybe just always return SparseMatrix?

Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

Just a few minor changes. 👍

base/linalg/uniformscaling.jl Outdated Show resolved Hide resolved
base/linalg/uniformscaling.jl Outdated Show resolved Hide resolved
@mbauman
Copy link
Member

mbauman commented Oct 27, 2017

Interestingly, this should also work for negative indices. At first I thought about requesting a bounds check there, but given that I should work similarly well for OffsetArrays… as should this definition as-is… I think we could leave that ability as a feature. Thoughts?

@dalum
Copy link
Contributor Author

dalum commented Oct 30, 2017

@mbauman Thanks for the review! ❤️ That it works for negative indices was intentional, since integer indexing works that way. Any other infinite-dimensional abstract matrix could also allow arbitrary integer indexing, so I see no good argument for enforcing strictly positive indexing.

@stevengj
Copy link
Member

stevengj commented Apr 9, 2020

Do we want to resurrect this at some point?

It occurred to me recently that it might be nice to have the related syntax I[1:m, n] for forming the n-th unit vector (as a Vector).

@mbauman mbauman dismissed their stale review April 10, 2020 16:45

review was addressed!

@dalum
Copy link
Contributor Author

dalum commented May 30, 2020

@stevengj I updated the PR to include the indexing you suggested. I also had to change the behaviour of range indexing to return a dense array, since the separation of LinearAlgebra and SparseArrays meant that sparse functions were not available in the scope. I think it's better not to make LinearAlgebra depend on SparseArrays for this functionality?

The implementation right now is about twice as fast as first instancing a diagonal identity matrix through I(n) and then subsequently indexing into it, at least for small matrices.

@stevengj
Copy link
Member

stevengj commented Jun 1, 2020

Needs a NEWS item?

@mbauman mbauman added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change labels Jun 1, 2020
@dalum
Copy link
Contributor Author

dalum commented Jun 1, 2020

@mbauman It is not entirely clear to me where to add the compat annotation. There are no existing docstrings for getindex(::UniformScaling), so should I write one? I'm just a little worried about cluttering ?getindex.

@mbauman
Copy link
Member

mbauman commented Jun 1, 2020

Ah, very good question. I agree about it not really belonging in getindex, but I think we could say something in the type itself:

Generically sized uniform scaling operator defined as a scalar times the
identity operator, `λ*I`. See also [`I`](@ref).

It could be nice to say something about it acting somewhat matrix-like with some indexing supported but without an explicit size — and then a compat entry would fit there.

@mbauman mbauman removed the needs news A NEWS entry is required for this change label Jun 1, 2020
@dalum
Copy link
Contributor Author

dalum commented Jun 1, 2020

Thanks! Not sure if I worded it right, but I added a docstring example that should give some idea about the kind of indexing that is supported.

@mbauman
Copy link
Member

mbauman commented Jun 1, 2020

Looks great to me, thank you!

@mbauman mbauman added linear algebra Linear algebra and removed needs compat annotation Add !!! compat "Julia x.y" to the docstring labels Jun 1, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #24359 into master will increase coverage by 0.02%.
The diff coverage is 96.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   JuliaLang/julia#24359      +/-   ##
==========================================
+ Coverage   86.12%   86.14%   +0.02%     
==========================================
  Files         348      348              
  Lines       64939    64971      +32     
==========================================
+ Hits        55928    55970      +42     
+ Misses       9011     9001      -10     
Impacted Files Coverage Δ
base/multimedia.jl 50.00% <ø> (ø)
stdlib/Test/src/Test.jl 89.74% <75.00%> (-0.13%) ⬇️
stdlib/SparseArrays/src/sparsevector.jl 95.26% <88.88%> (-0.07%) ⬇️
stdlib/SparseArrays/src/sparsematrix.jl 89.68% <91.66%> (+<0.01%) ⬆️
base/show.jl 88.53% <100.00%> (ø)
stdlib/SparseArrays/src/linalg.jl 95.14% <100.00%> (+0.36%) ⬆️
base/reshapedarray.jl 86.15% <0.00%> (-0.77%) ⬇️
base/bitset.jl 93.20% <0.00%> (ø)
base/missing.jl 88.52% <0.00%> (+0.54%) ⬆️
... and 1 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 7301dc6...e4d4762. Read the comment docs.

@stevengj stevengj merged commit cbd854b into JuliaLang:master Jun 6, 2020
@dalum dalum deleted the evey/unindex branch June 7, 2020 17:38
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
* Add range indexing to UniformScaling

* Add tests

* Return sparse matrix when indexing UniformScaling

* start -> first

* Correctly handle non-integer steps

* Make getindex(::UniformScaling, ranges...) return a dense array

* Remove unneccessary (and slower) branch

* Add (range, integer) indexing to UniformScaling

* Update tests

* Fix unbound param

* Add NEWS item

* Update docstring and add compat annotation

Co-authored-by: Evey Dee <eveydee@users.noreply.github.com>
@stevengj
Copy link
Member

Should we make I(m,n) return I[1:m, 1:n]?

@StefanKarpinski
Copy link
Member

Seems like the only sane definition to me, so 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants