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 {ones|zeros}(A::AbstractArray[, opts...]) methods #24656

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Nov 19, 2017

This pull request deprecates ones(A::AbstractArray[, opts...]) and zeros(A::AbstractArray[, opts...]) methods. Ref. #24595 for background. Best!

(CI failures anticipated. Will remove the WIP tag after working the CI failures out.)

@Sacha0 Sacha0 added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Nov 19, 2017
@Sacha0 Sacha0 changed the title deprecate {ones|zeros}(A::AbstractArray[, opts...]) methods [WIP] deprecate {ones|zeros}(A::AbstractArray[, opts...]) methods Nov 19, 2017
NEWS.md Outdated
@@ -434,6 +434,9 @@ Deprecated or removed
* `expand(ex)` and `expand(module, ex)` have been deprecated in favor of
`Meta.lower(module, ex)` ([#22064, #24278]).

* `ones(A::AbstractArray[, opts...])` and `zeros(A::AbstractArray[, opts...])` methods
have been deprecated ([#24656]).
Copy link
Contributor

Choose a reason for hiding this comment

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

in favor of fill!(similar(A[, opts...]), 0/1)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! And done! Thanks! :)

@Sacha0 Sacha0 force-pushed the depozarr branch 3 times, most recently from 08f1352 to bf56cac Compare November 20, 2017 04:30
@Sacha0 Sacha0 changed the title [WIP] deprecate {ones|zeros}(A::AbstractArray[, opts...]) methods deprecate {ones|zeros}(A::AbstractArray[, opts...]) methods Nov 20, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 20, 2017

Absent objections or requests for time, I plan to merge these changes this evening (PT) or later. Best!

@jebej
Copy link
Contributor

jebej commented Nov 20, 2017

It doesn't seem clear why these methods need to be deprecated. Why remove convenience and what does this accomplish/make better?

I went through the discussion in #24595, and the explanations given there (generic array types, element types) do not seem to apply here, where you just want the same array and element types.

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 20, 2017

In short, these methods see little if any use in the wild, and triage seemingly unanimously questioned their general soundness/wisdom. Best!

@StefanKarpinski
Copy link
Member

The issue is that the methods with array arguments or array type arguments are meant to be used in fairly generic code, but in generic code there are always better options than zeros or ones such as fill and fill! with an actual one or zero value of the appropriate type.

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 20, 2017

Thanks @StefanKarpinski for the more complete response :).

@jebej
Copy link
Contributor

jebej commented Nov 20, 2017

If you have an array A of a particular type, and you just want a new array of zeros of that type, zeros(A) is simple and generic. What would the better option be?

I also still don't see problems with these methods, and thus reasons to remove them. That there are more generic methods available is an argument for using those other methods if needed, but not remove the convenient ones.

I don't have strong feelings about this one, but am commenting here regarding the direction arrays have taken recently. I agree that we need methods that are more generic to generate different MyArray types in a uniform way, but removing convenience methods for the basic array types does not seem necessary to achieve that. We could have both powerful and generic constructors MyArray{T,N}(1.0I,n,m) and simple ones like zeros(m,n) coexist peacefully.

Thanks for the hard work.

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 20, 2017

These methods suffer from some of the shortcomings described in #24595. Additionally, these methods are not well defined in general: Should ones(S::Diagonal) return a Diagonal with ones on the diagonal, or a matrix full of ones? Given the latter choice, should that matrix be a Matrix, or a SparseMatrixCSC? If you specify shape as in ones(S::Diagonal, shape...), what should that call return in general (given that you can only return a Diagonal for square shape, and if the return type depends on shape then you lose type stability)? If you specify element type as in ones(S::Diagonal, T), should the result contain one(T)s or oneunit(T)s, and what do you do when one(T) isn't of type T? What about the same questions for Bidiagonal, Tridiagonal, and SymTridiagonal? What should ones(S::AbstractSparseArray) do, return a copy of S with its stored entries filled with ones/oneunits, or an instance of typeof(S) containing a one/oneunit at every addressable entry? And so forth... :).

@jebej
Copy link
Contributor

jebej commented Nov 21, 2017

  • I wouldn't use zeros or ones with Diagonal or those other more complex types, and I would point out that the exact same ambiguity exists with fill!(copy(A::Diagonal),a) (does filling a diagonal matrix result in a diagonal matrix?). However, for consistency, if fill!(copy(A::Diagonal),a) fills only the diagonal, which it does, the same should go for ones and zeros.
  • You can't pass a shape to fill!, so it doesn't seem like an issue not have those methods for zeros and ones. (i.e. replacing with fill! does not help that case)

For the rest, like I said already, I am not suggesting to remove fill!, and so those more complex cases can be handled there... I am only trying to preserve convenience in simple cases, which I would hope people still think has value.

@jebej
Copy link
Contributor

jebej commented Nov 21, 2017

I won't argue further, if the desire to remove those methods is strong, I'll just do the substitution in my code.

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 21, 2017

I would point out that the exact same ambiguity exists with fill!(copy(A::Diagonal),a) (does filling a diagonal matrix result in a diagonal matrix?). However, for consistency, if fill!(copy(A::Diagonal),a) fills only the diagonal, which it does, the same should go for ones and zeros.

No such ambiguity exists with fill!(D::Diagonal, val): (1) fill! mutates and returns its first argument, so there is no question of whether fill!(D::Diagonal, val) returns something other than a Diagonal. (2) fill!(D::Diagonal, val)'s semantics are now (on master) well-defined: For structurally constrained storage types, fill!(A, val) throws / will throw for nonzero val, and the fill! methods that did otherwise have been / will be deprecated in favor of fillslots! (which is being renamed fillstored!).

I wouldn't use zeros or ones with Diagonal or those other more complex types

If not, then one or more of fill(val, size(A)), fill!(copy(A), val), or {ones|zeros}(size(A)) (edit: or zero(A), thanks @Jutho!) works just as well, and in contrast the former two are clear and well-defined. If you are not using these methods' genericity, why do you need that genericity around?

You can't pass a shape to fill!, so it doesn't seem like an issue not have those methods for zeros and ones. (i.e. replacing with fill! does not help that case)

You cannot pass a shape to fill! as, as above, fill! does not allocate a new object; comparing fill! and ones/zeros makes little sense here. Rather, fill is the relevant generalization of ones/zeros, and fill does accept a shape argument. Best!

@Jutho
Copy link
Contributor

Jutho commented Nov 21, 2017

@jebej, note that instead of zeros(A), what is available to obtain fill!(copy(A),0) is just plain and simple zero(A), i.e. the identity element for addition of arrays of the same size.

@StefanKarpinski
Copy link
Member

@jebej, I think we all appreciate that this is a somewhat unintuitive change because ones and zeros seem so obvious. I think the element of surprise stems from how problematic they actually turn out to be in writing generic code. Hope it helps that you are not alone in finding it not immediately obvious that they are a problem.

@jebej
Copy link
Contributor

jebej commented Nov 21, 2017

@jebej, note that instead of zeros(A), what is available to obtain fill!(copy(A),0) is just plain and simple zero(A), i.e. the identity element for addition of arrays of the same size.

That's true, thanks, as that takes care of the simple case I was worried about, although only for zeros since one(A) is the identity matrix, and not the Hadamard multiplicative identity like ones.

I think the element of surprise stems from how problematic they actually turn out to be in writing generic code

Like I said above, if only the simple case is needed, then ones(A) is generic.

Again, this is not a major issue for me, and I appreciate the amount of thought going to having a coherent API that is both powerful/extensible and simple.

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 21, 2017

News item updated with other possible rewrites. Best!

@Sacha0 Sacha0 merged commit ea34968 into JuliaLang:master Nov 22, 2017
@Sacha0 Sacha0 deleted the depozarr branch November 22, 2017 04:28
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 22, 2017

Thanks all! :)

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 26, 2017

Great additional discussion starting #24444 (comment). Best!

@deprecate ones(a::AbstractArray, ::Type{T}, dims...) where {T} fill!(similar(a, T, dims...), 1)
@deprecate ones(a::AbstractArray, ::Type{T}) where {T} fill!(similar(a, T), 1)
@deprecate ones(a::AbstractArray) fill!(similar(a), 1)
@deprecate zeros(a::AbstractArray, ::Type{T}, dims::Tuple) where {T} fill!(similar(a, T, dims), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

For dimensionful numbers, the replacement method doesn't work. You would need zero(T) instead of 0 since converting between dimensionful and dimensionless numbers is not allowed. Likewise for ones, probably you want oneunit(T) instead of 1.

Unless there are other considerations for using the literal numbers, shall I could open a PR to address this?

Copy link
Member

Choose a reason for hiding this comment

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

Likely an oversight since the methods these replace used zero(T) and one(T). Feel free to submit a PR 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

A pull request fleshing out these deprecations would be lovely! Including the alternative rewrites mentioned in this pull request's news entry would be wonderful as well.

@stevengj
Copy link
Member

Is the plan to also deprecate zeros(dims...) in favor of fill(0.0, dims...) etcetera?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 12, 2017

The winds of triage most recently favored the compromise of deprecating all remaining ones/zeros methods exceptones(dims...)/zeros(dims...). (I favor deprecating those methods in addition.) Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants