JuliaDynamics / RecurrenceAnalysis.jl

Recurrence Quantification Analysis in Julia
https://juliadynamics.github.io/RecurrenceAnalysis.jl/stable/
MIT License
45 stars 12 forks source link

default theiler=1; 0 for cross-recurrence matrices #51

Closed heliosdrm closed 5 years ago

heliosdrm commented 5 years ago

Closes #45

It was a bit more difficult than what I expected, because redefining the methods for CrossRecurrenceMatrix with only a change in keyword arguments led me to recursive calls!

Finally I have achieved it using a "dummy" type _RM<:AbstractRecurrenceMatrix

https://github.com/JuliaDynamics/RecurrenceAnalysis.jl/blob/65b23c6a147c27e591f1a6ed1ad1ce9e9041f2b5/src/matrices.jl#L104-L108

But perhaps there is a smarter solution that I missed...

Datseris commented 5 years ago

Huh....

Isn't it just super simpler to define the function:

deftheiler(x) = 1
deftheiler(x::CrossRecurrenceMatrix) = 0

and in all functions replace every theiler = 0 with thelier = deftheiler(x)? Julia allows keywords to depend on previous arguments and the recurrence matrix is always an argument...

I believe the code became unnecessarily complicated, and this looks very ugly:

# Special type (not exported) to manage conditional parameters in methods
struct _RM <: AbstractRecurrenceMatrix
    data::SparseMatrixCSC{Bool,Int}
end
_RM(R::ARM) = _RM(R.data)

could you please explain why my simple suggestion isn't enough?

heliosdrm commented 5 years ago

Yes, it is enough. I just was not smart enough to think on that! :flushed:

Datseris commented 5 years ago

Yes, it is enough. I just was not smart enough to think on that! 😳

Heh, no worries! Making this package took a lot of brainpower! These things are easy to overlook after writing so much code!

So we agree, right? Do you want me to do these changes?

heliosdrm commented 5 years ago

Well, I did it already.