fix breaking zscore changes (backport to 0.32 branch) #565
Conversation
Codecov Report
@@ Coverage Diff @@
## v0.32-backport #565 +/- ##
==================================================
- Coverage 90.59% 90.47% -0.13%
==================================================
Files 21 21
Lines 2244 2246 +2
==================================================
- Hits 2033 2032 -1
- Misses 211 214 +3
Continue to review full report at Codecov.
|
Co-Authored-By: Milan Bouchet-Valat <[email protected]>
src/transformations.jl
Outdated
| @@ -131,6 +137,7 @@ function fit(::Type{ZScoreTransform}, X::AbstractVector{<:Real}; | |||
| dims::Union{Integer,Nothing}=nothing, center::Bool=true, scale::Bool=true) | |||
| if dims == nothing | |||
| Base.depwarn("fit(t, x) is deprecated: use fit(t, x, dims=2) instead", :fit) | |||
| dims = 1 | |||
There was a problem hiding this comment.
Woops, shouldn't it be:
| dims = 1 | |
| dims = 2 |
|
That will just result in an error. Depends how pedantic we want to be about the default being 2. I'd argue that that would still be breaking since code that worked before would result in an error.
… On Mar 2, 2020, at 12:58, Milan Bouchet-Valat ***@***.***> wrote:
@nalimilan commented on this pull request.
In src/transformations.jl:
> @@ -131,6 +137,7 @@ function fit(::Type{ZScoreTransform}, X::AbstractVector{<:Real};
dims::Union{Integer,Nothing}=nothing, center::Bool=true, scale::Bool=true)
if dims == nothing
Base.depwarn("fit(t, x) is deprecated: use fit(t, x, dims=2) instead", :fit)
+ dims = 1
Woops, shouldn't it be:
⬇️ Suggested change
- dims = 1
+ dims = 2
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
If there's a relatively simple way of making the old code work, then better implement it I guess. |
|
Sorry I think I misunderstood your point. Were you replying to my last suggestion? Anyway, AFAICT before #490 we didn't support standardizing vectors, so that probably doesn't matter. |
|
I'm not exactly sure whether vectors had first class support but there were transform etc. methods for `Array{<:Real, 1}` so that should still work to avoid breakage. But maybe I'm misunderstanding how vectors were handled before #490.
… On Mar 3, 2020, at 02:30, Milan Bouchet-Valat ***@***.***> wrote:
Sorry I think I misunderstood your point. Were you replying to my last suggestion? Anyway, AFAICT before #490 we didn't support standardizing vectors, so that probably doesn't matter.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Ah, yeah, the situation was weird at the time. Well, any deprecation is better than the current state, so choose the one you think is best. |
|
yeah now that I'm looking back at the state of things in v0.32 was pretty strange...I think providing a no-dims method that does something reasonable and provides a deprecation warning is a reasonable compromise between being a stickler about not changing the API at all (e.g., used to error on vector, still errors on a vector) and making the merged stuff useful. |
|
Actually, I changed my mind. Previously (v0.32) |
fixes #556
#490 introduced breaking changes to the constructor of the ZScoreTransform type, adding a fourth mandatory argument. It also introduced an error in cases where the previous default (without
dims=) was used forfitand everything the calls it. This PR fixes those by adding an inner constructor with a depwarn for ZScoreTransform, and re-setting the default dims inside the call tofitwhendims=nothing.