Skip to content

Conversation

@heliosdrm
Copy link
Collaborator

To deal with #106 and the last comments on #102

With this, using julia --depwarn=yes:

julia> using RecurrenceAnalysis
julia> x=randn(100, 2);
julia> RecurrenceMatrix(x,0.4)
┌ Warning: `RecurrenceMatrix(x::AbstractMatrix, ε; kwargs...)` is deprecated, use `RecurrenceMatrix(Dataset(x), ε; kwargs...)` instead.

But the calculations go as if the matrix had been converted to a Dataset. (By default, in Julia 1.5+ the deprecation warning is hidden).

If this is approved, src/deprecate.jl could be extended later on for RecurrenceMatrix{FAN} etc.

@heliosdrm heliosdrm requested a review from Datseris December 1, 2020 10:48
@heliosdrm
Copy link
Collaborator Author

My doubt is: do we want to keep the deprecation warning as it is by default (silent unless you activate the option depwarn=yes, which most people won't do), or is it better to make it more prominent, and always throw the warning to instruct users that Dataset must be used for multi-dimensional data?

@Datseris
Copy link
Member

Datseris commented Dec 1, 2020

I think the depwarning always has to be thrown. How did you make it "not being thrown by default"? When I use @deprecate or @warn the are always shown when starting Julia the default way.

@heliosdrm
Copy link
Collaborator Author

How did you make it "not being thrown by default"? When I use @deprecate or @warn the are always shown when starting Julia the default way.

I did not make it. It's the default as of Julia 1.5:
https://docs.julialang.org/en/v1/base/base/#Base.@deprecate

@Datseris
Copy link
Member

Datseris commented Dec 2, 2020

Okay then it is an easy change. Instead of using @deprecate we can instead use @warn inside the function call.

@heliosdrm
Copy link
Collaborator Author

@warn would repeat the message every time matrices are used as inputs, whereas @deprecate would show the message only the first time it happens. Also, according to the answers in Discourse in 1.6 we would be able to force the deprecation message.

Should we make it a regular warning, instead of a deprecation warning, still?

@Datseris
Copy link
Member

Datseris commented Dec 3, 2020

@warn would repeat the message every time matrices are used as inputs

That's what I always do actually. Don't know if it is good practice or not, but if I know there is some code that will break in the next release, I'd rather press the user to change it as soon as possible instead of passively letting it go after 1 warning.

(this must be revised later on when DelayEmbeddings is updated to 2.0)
@Datseris Datseris merged commit 2511090 into master Dec 4, 2020
@Datseris Datseris deleted the deprecate_AbstractMatrix branch December 4, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants