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

deprecate fill!(A::[Diagonal|AbstractTriangular], x) = fillslots!(A, x) methods #24413

Merged
merged 1 commit into from
Nov 1, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 30, 2017

This pull request deprecates two methods, fill!(A::S, x) = fillslots!(A, x) for S in (Diagonal, AbstractTriangular). As the comment attached to the associated definitions states, these methods are an artifact of history.

Should we consider deprecating fill!(A, x) for structured A as well? Unless x is zero or A is small enough that all entries are mutable (i.e. A is one-by-one or two-by-two depending on the special matrix type), fill!(A, x) errors.

Best!

@Sacha0 Sacha0 added deprecation This change introduces or involves a deprecation linear algebra Linear algebra needs news A NEWS entry is required for this change labels Oct 30, 2017
@fredrikekre
Copy link
Member

LoadError("sysimg.jl", 438, LoadError("deprecated.jl", 1884, UndefVarError(:AbstractTriangular)))

@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label Oct 30, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 30, 2017

LoadError...

Should be fixed. Thanks! :) Thoughts re. structured fill!? Best!

@fredrikekre
Copy link
Member

Looks like this testset

@testset "fill! and fillslots!" begin
needs some work too.

Thoughts re. structured fill!? Best!

I agree that these methods are a bit weird. Although, it is convenient to zero out an array with fill! for generic code. Consider a function that will add elements to the diagonal. In theory this function can accept Matrix or Diagonal, since both of these allows for setindex! on the diagonal indices. But there is no common function that can "reset" the matrix by filling with 0. I have not had this problem with Diagonal and friends, but the same thing applies to Matrix vs Symmetric{Matrix} where I often have to add something like isa(A, Symmetric) ? fill!(A.data, 0) : fill!(A, 0) which is not very generic and kind of ugly. One option would be to have a fallback fillslots!(A, x) = fill!(A, x) so that fillslots! work on more containers, and the other option is to allow fill!(A, x) iff x == 0 for Diagonal, Symmetric etc.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

The linalg/bidiag tests should be fixed. Thanks for isolating that failure! :)

I agree that these methods are a bit weird. Although, it is convenient to zero out an array with fill! for generic code.

Agreed, the convenience of fill!(A, 0) is my one hesitation.

One option would be to have a fallback fillslots!(A, x) = fill!(A, x) so that fillslots! work on more containers

Coincidentally I have work in progress extending fillslots! to additional types :). Best!

@fredrikekre
Copy link
Member

Agreed, the convenience of fill!(A, 0) is my one hesitation.

Perhaps time to reconsider #19912?

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

Perhaps time to reconsider #19912?

I tend to agree that reducing rather than expanding such redundancy is preferable :).

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

Absent objections or requests for time, I plan to merge these changes tomorrow. Best!

@Sacha0 Sacha0 merged commit b6a2394 into JuliaLang:master Nov 1, 2017
@Sacha0 Sacha0 deleted the depoddfillbang branch November 1, 2017 23:53
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 1, 2017

Thanks all! :)

NHDaly added a commit to NHDaly/julia that referenced this pull request Aug 30, 2018
The History.md file linked to JuliaLang#24413 for `eye` deprecation in 0.7. This commit changes it to link to JuliaLang#24415.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants