Closed JanJereczek closed 3 months ago
Yes, open an issue for Mann-Kendall correlation. No, it is not mandatory to pass the review.
FOr the offset-arrays: I don't understand your argument. What do you mean we want to have "normal" indexing? The indexing of an array has nothing to do with its values. You can have non-equispaced data in an OffsetArray
. The time vector would be such an array.
It is very easy to solve the issue. We just change the code in this file https://github.com/JuliaDynamics/TransitionsInTimeseries.jl/blob/main/src/misc/timeseries.jl to instead of use 1
to use fi = firstindex(t)
. And instead of use 2, 3
to use fi + 1, fi + 2
. We should just fix it, it's not much code change.
The
.csv
data used for Figs. 1 and 2 can now be generated fromewstools
by runningpaper/code/ewstools-tuto-1.py
. This addresses
Do we cite the version of ewstools
? Only then the csv file can be re-generated.
The functions of src/misc/timeseries.jl
now support OffsetArrays.
The version of ewstools
used for benchmarking is now specified in paper.md
and a corresponding check is performed in paper/code/ewstools-tuto-1.py
.
Finally, I noticed our test environment had unused (heavy) packages, which I removed.
great, thanks! Would you mind writing a reply message in the JOSS review GitHub issue?
Yes, I just left work so let me do this tomorrow!
On Wed, 22 May 2024, 17:59 George Datseris, @.***> wrote:
great, thanks! Would you mind writing a reply message in the JOSS review GitHub issue?
— Reply to this email directly, view it on GitHub https://github.com/JuliaDynamics/TransitionsInTimeseries.jl/pull/83#issuecomment-2125152303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ3TCWT3AKEWU5KCFV5JE6DZDS6HXAVCNFSM6AAAAABIBWFBWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGE2TEMZQGM . You are receiving this because you authored the thread.Message ID: @.***>
Hi @Datseris!
This PR addresses most comments made by the reviewers for the JOSS publication. In particular:
.csv
data used for Figs. 1 and 2 can now be generated fromewstools
by runningpaper/code/ewstools-tuto-1.py
. This addresses #81.@belapsed
and the python code has been adapted accordingly (extract min instead of mean runtime). This addresses #80.tail = :right
, corrected:tail = (:right, right)
). This addresses #79.paper.md
.This only leaves two points open:
OffsetArray
does not work withequispaced_step
, as mentioned in #78. However, I think this is not a problem, since we want to have normal indexing, even for timeseries that have a variabledt
. Your opinion is welcome!Additionally, this PR permutes the left and right axes used in the plotting function. Looking at it again after a while, it appeared much more readable to me in this new version.