Some adjustments for dimensionful numbers #25035
Conversation
fredrikekre
left a comment
fredrikekre
left a comment
There was a problem hiding this comment.
None of the methods should call oneunit(T), the deprecations should exactly mimic the old behavior, which was to call one(T) in all the cases here.
ajkeller34
force-pushed
the
dimensionsfix
branch
from
8e6276d to
fdc9e67
Compare
|
Yes, that's right. I've updated the PR. |
|
Thanks @ajkeller34! :) I hope to have a look late this evening PT or tomorrow evening PT. Best! |
NEWS.md
Outdated
| `zero(A)`. For `ones(A)` or `zeros(A)`, consider `fill(v, size(A))` for `v` an | ||
| appropriate one or zero, `fill!(copy(A), {1|0})`, `ones(size(A))` or | ||
| `zeros(size(A))`, or any of the preceding with different element type | ||
| appropriate one or zero, `fill!(copy(A), {one(eltype(A)) | zero(eltype(A))})`, |
There was a problem hiding this comment.
Perhaps leave fill!(copy(A), {1|0}) where it is, add fill!(similar(A), {1|0}) shortly thereafter, and only later suggest fill!(copy(A), {one(eltype(A)) | zero(eltype)}) for where strictly necessary? Providing the simplest possible replacements that will work in most cases first seems best.
base/deprecated.jl
Outdated
| @deprecate zeros(a::AbstractArray, ::Type{T}, dims::Tuple) where {T} fill!(similar(a, T, dims), zero(T)) | ||
| @deprecate zeros(a::AbstractArray, ::Type{T}, dims...) where {T} fill!(similar(a, T, dims...), zero(T)) | ||
| @deprecate zeros(a::AbstractArray, ::Type{T}) where {T} fill!(similar(a, T), zero(T)) | ||
| @deprecate zeros(a::AbstractArray) fill!(similar(a), zero(eltype(a))) |
There was a problem hiding this comment.
With benefit of hindsight, ideally each of these deprecations would throw a longer depwarn, containing similar information to that in the news entry. I conjecture most users will see only the depwarn, and the suggestions in the news entry that are simpler than the depwarn are likely to be much better received than the general replacement. Thoughts? :)
Sacha0
left a comment
Sacha0
left a comment
There was a problem hiding this comment.
Looks great modulo the minor review comments! Thanks @ajkeller34! :)
(The second review comment is aspirational. If you are not excited about addressing it, please do not feel obligated! I can make that change in a later pull request. Also, apologies for the slower than expected review!)
ajkeller34
force-pushed
the
dimensionsfix
branch
from
fdc9e67 to
75f80a7
Compare
|
Sorry for the delay (holidays). I rebased and tried to address the minor review comments:
|
Sacha0
left a comment
Sacha0
left a comment
There was a problem hiding this comment.
These changes look great! Thanks @ajkeller34! :)
4 participants
In an effort to restore compatibility of Unitful.jl with the nightly build, I noticed some of the tests started failing following PR #24656. In that PR, literal numbers
0and1were used instead ofzero(T)andoneunit(T), causing issues for dimensionful numbers. This PR substitutes inzero(T)andoneunit(T)and updates NEWS.md with revised guidance (although the guidance is even more verbose than before...)Cross-ref #24656 (comment)