search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
714 stars 38 forks source link

[REVIEW]: Multitaper.jl: A Julia package for frequency domain analysis of time series #2463

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @lootie (Charlotte Haley) Repository: https://bitbucket.org/clhaley/Multitaper.jl Version: 1.1.0 Editor: @VivianePons Reviewers: @anowacki, @agricolab Archive: 10.5281/zenodo.4274772

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/12835715797572f71ac49840d12d4fe6"><img src="https://joss.theoj.org/papers/12835715797572f71ac49840d12d4fe6/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/12835715797572f71ac49840d12d4fe6/status.svg)](https://joss.theoj.org/papers/12835715797572f71ac49840d12d4fe6)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@anowacki & @agricolab, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @VivianePons know.

✨ Please try and complete your review in the next six weeks ✨

Review checklist for @anowacki

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @agricolab

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @daviddewhurst (partial review)

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @felixcremer (partial review)

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @daviddewhurst, @felixcremer it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1109/LSP.2017.2719943 is OK
- 10.1109/ACSSC.2011.6190385 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1093/gji/ggz280 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1017/jfm.2018.366 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1016/0098-3004(94)00067-5 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1016/j.cageo.2008.06.007 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1007/s13137-011-0016-z is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1098/rspa.2014.0101 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1109/proc.1982.12433 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1109/jproc.2007.894712 is INVALID because of 'https://doi.org/' prefix
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

VivianePons commented 4 years ago

I am copying here the last comment made by @daviddewhurst on the pre-review issue to be sure it's not lost in the process:

@lootie I'm currently not able to install the package as directed by README.md.:

(v1.1) pkg> add git://bitbucket.org/clhaley/Multitaper.jl.git Updating registry at ~/.julia/registries/General Updating git-repo https://github.com/JuliaRegistries/General.git Cloning git-repo git://bitbucket.org/clhaley/Multitaper.jl.git ERROR: failed to clone from git://bitbucket.org/clhaley/Multitaper.jl.git, error: GitError(Code:ERROR, Class:OS, failed to connect to bitbucket.org: Operation timed out)

However, using add https://bitbucket.org/clhaley/Multitaper.jl.git works fine. Just a heads-up for you (and other reviewers / editor) that they might encounter this as well.

VivianePons commented 4 years ago

Just to let you know, I am taking some time off until August 24, I will still be checking my emails and keep an eye on my current assigned submissions. I might just take a bit more time than usual to answer.

lootie commented 4 years ago 
@whedon generate pdf
VivianePons commented 4 years ago

Dear all,

where are we on this?

VivianePons commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

VivianePons commented 4 years ago

Hi @felixcremer and @daviddewhurst do you think you'll have time to work on this review soon enough?

Do not hesitate to post comments to the author even if your review is not finished

daviddewhurst commented 4 years ago

Hi @VivianePons, my review will be complete by Sunday.

lootie commented 4 years ago

Hi @VivianePons @felixcremer @daviddewhurst I am happy to answer any questions you have or make changes based on your suggestions. Since the submission of this article, I have added additional functionality for time series with missing data, and I welcome any comments on this as well.

VivianePons commented 4 years ago

Hi @daviddewhurst and @felixcremer I see no progress on the review, have you had time to look at the software? When do you think you can get back to it?

Best

Viviane

lootie commented 4 years ago

@VivianePons perhaps @daviddewhurst and @felixcremer have unsubscribed to notifications from this thread?

VivianePons commented 3 years ago

Indeed, I just an email to both of them. Thank you for your patience!

lootie commented 3 years ago

@VivianePons Is there anything I can do to get this review underway, short of submitting it elsewhere? I believe our work checks all the boxes above, but I've received no feedback so far.

VivianePons commented 3 years ago

I am very sorry for this. Both of the reviewers who had accepted to take this review have been completely unresponsive, both through github and via email, even though they both started the review this summer. This is quite a rare occurrence, especially to happen to both reviewers at the same time. I have been quite busy myself and let time run more than I should have. But I will email new reviewers this week and hopefully, we can get this rolling.

Thank you again for your patience. I hope you have a better review experience from now on!

lootie commented 3 years ago

Thank you for your help. I suggested five potential reviewers here on Jul 2 if that helps you assign someone new. @VivianePons

VivianePons commented 3 years ago

I have just sent 2 new review invites using your initial list of potential reviewers. We'll see how it goes.

@daviddewhurst and @felixcremer I would still be happy to get your reviews!

VivianePons commented 3 years ago

@whedon add @anowacki as reviewer

whedon commented 3 years ago

OK, @anowacki is now a reviewer

anowacki commented 3 years ago

Hi @lootie. I'm just starting to take a look and can see a few things where I would ideally open an issue at the repo (which I think we're encouraged to do). However, I can't see any way to do this for your repo on Bitbucket. (This is also what the current proof of the paper suggests people do.)

Do you know if there's any way to enable issues? Otherwise I will just note these issues in posts here.

lootie commented 3 years ago

Thank you @anowacki for looking at this right away. I enabled the issue tracker, so you are free to put your comments there if you'd like. If the issue prevents you from checking one of the boxes in your review above, please put it in this thread and I'll address it right away.

anowacki commented 3 years ago

@lootie Thanks for turning that on—I will submit issues to the repo.

It looks like I can set a priority for them, so I will do that and mark anything as a blocker to ticking the boxes above as 'Blocker'. Things which are suggestions I will mark ~'Minor'~ as an 'enhancement'. There are a few things I think I will have to ask you to do, so probably doing it that way will be easier to keep track of.

lootie commented 3 years ago

@anowacki Thank you for your detailed comments! I am working my way through the issues now, and expect to get through them all by Monday.

lootie commented 3 years ago

To update on progress, I've implemented all but two of the blockers (continuous integration will take some thought), and expect to complete the issues marked "major" by the end of the week. Will update again when these are ready.

agricolab commented 3 years ago

@VivianePons , I can do the review, as requested by your mail today.

VivianePons commented 3 years ago

Thank you! I am adding you as a reviewer

VivianePons commented 3 years ago

@whedon add @agricolab as reviewer

whedon commented 3 years ago

OK, @agricolab is now a reviewer

lootie commented 3 years ago

To update this thread, @anowacki I have implemented all but two (or three) of the comments.

Issue 18 I have worked on using Documenter.jl to the point where one can compile the documentation (though it is rudimentary now), but hosting from Bitbucket appears to be a non-standard thing. I will try to work around this and update on Monday. Issue 22 I will complete this on the weekend too. Issue 17 Coverage - will need to update test file to increase the coverage. Issue 15 ditto to updating test file.

@agricolab Feel free to submit your review in the form of issues, or in this thread.

agricolab commented 3 years ago

I am currently in the process of reviewing, and will open issues in the main repo on bitbucket in the course of this week.

agricolab commented 3 years ago

I assume that @lootie is indeed run by Charlotte Haley, there is no identifying information on this github account. Under these conditions, @lootie did contribute to the repository, as required by the general checks on contribution and authorship.

agricolab commented 3 years ago
agricolab commented 3 years ago

Test Summary: | Pass Total univariate multitaper | 24 24 Test Summary: | Pass Total crossings multitaper | 3 3 Test Summary: | Pass Total mdmwps multitaper | 9 9 multivariate multitaper: Error During Test at /media/user/sd/tools/julia/multitaper.jl/test/runtests.jl:13 Got exception outside of a @test LoadError: MethodError: no method matching Array{Int64,1}(::Float64) Closest candidates are: Array{Int64,1}() where T at boot.jl:425 Array{Int64,1}(!Matched::UndefInitializer, !Matched::Int64) where T at boot.jl:406 Array{Int64,1}(!Matched::UndefInitializer, !Matched::Int64...) where {T, N} at boot.jl:412 ... Stacktrace: [1] freq_to_int(::Float64, ::Int64, ::Float64) at /home/user/.julia/packages/Multitaper/cPkHj/src/Utils.jl:71 [2] (::Multitaper.var"#28#30"{Float64,Int64})(::Int64) at /home/user/.julia/packages/Multitaper/cPkHj/src/Univariate.jl:306 [3] map!(::Multitaper.var"#28#30"{Float64,Int64}, ::Array{Float64,1}, ::Base.OneTo{Int64}) at ./abstractarray.jl:2155 [4] multispec(::Array{Float64,1}; NW::Float64, K::Int64, dt::Float64, ctr::Bool, pad::Float64, dpVec::Array{Float64,2}, egval::Nothing, guts::Bool, a_weight::Bool, Ftest::Bool, highres::Bool, jk::Bool, Tsq::Array{Float64,1}, alph::Float64) at /home/user/.julia/packages/Multitaper/cPkHj/src/Univariate.jl:306 [5] (::Multitaper.var"#79#85"{Float64,Int64,Float64,Bool,Float64,Bool,Bool,Bool,Float64})(::Array{Float64,1}) at /home/user/.julia/packages/Multitaper/cPkHj/src/Multivariate.jl:313 [6] iterate at ./generator.jl:47 [inlined] [7] collect(::Base.Generator{Base.Generator{UnitRange{Int64},Multitaper.var"#80#86"{Array{Float64,2}}},Multitaper.var"#79#85"{Float64,Int64,Float64,Bool,Float64,Bool,Bool,Bool,Float64}}) at ./array.jl:686 [8] map at ./abstractarray.jl:2188 [inlined] [9] multispec(::Array{Float64,2}; outp::Symbol, NW::Float64, K::Int64, dt::Float64, ctr::Bool, pad::Float64, dpVec::Nothing, guts::Bool, a_weight::Bool, jk::Bool, Ftest::Bool, Tsq::Array{Float64,1}, alph::Float64) at /home/user/.julia/packages/Multitaper/cPkHj/src/Multivariate.jl:313 [10] top-level scope at /media/user/sd/tools/julia/multitaper.jl/test/multivariate_test.jl:9 [11] include(::String) at ./client.jl:457 [12] macro expansion at /media/user/sd/tools/julia/multitaper.jl/test/runtests.jl:14 [inlined] [13] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115 [inlined] [14] top-level scope at /media/user/sd/tools/julia/multitaper.jl/test/runtests.jl:14 [15] include(::Function, ::Module, ::String) at ./Base.jl:380 [16] include(::Module, ::String) at ./Base.jl:368 [17] exec_options(::Base.JLOptions) at ./client.jl:296 [18] _start() at ./client.jl:506 in expression starting at /media/user/sd/tools/julia/multitaper.jl/test/multivariate_test.jl:9

Test Summary: | Error Total multivariate multitaper | 1 1 ERROR: LoadError: Some tests did not pass: 0 passed, 0 failed, 1 errored, 0 broken. in expression starting at /media/user/sd/tools/julia/multitaper.jl/test/runtests.jl:12


Authors might want to fix automated test and/or add explicit test instructions.
lootie commented 3 years ago

Thank you, @agricolab, for your comments. I've made some changes below

I'll address the changes to the paper under "Statement of Need" and the bitbucket issue later today.

I would like to acknowledge @anowacki and @agricolab by name under the acknowledgements section of the manuscript, unless anyone objects. The feedback I've already received has been indispensable.

agricolab commented 3 years ago

Thanks. I just noticed my comment in brackets on user stories was wrongly formatted. github treated '<>' as html and ignored them. Iremoved them. I will also browse through your toolbox a little bit more in the next days.

lootie commented 3 years ago

To continue the message from earlier, @agricolab , I have added a user story to the paper and docs under "Statement of need" here. I've also added my email to the relevant documents for support requests, as required. I've changed the verbiage under "Contributions" so it is clear that submitting issues via bitbucket is welcome.

I have tried to match the test file error message above with a build error for one of the commits, but haven't been able to line them up. Perhaps the package was installed using Pkg> add https://bitbucket.org/clhaley/Multitaper.jl.git but then included a test file from a cloned version of the repo which is at a different commit? Pulling the latest commit, or using Pkg> test Multitaper from the REPL ought to work.

agricolab commented 3 years ago

Perhaps the package was installed using Pkg> add https://bitbucket.org/clhaley/Multitaper.jl.git but then included a test file from a cloned version of the repo which is at a different commit?

Yes! My fault, sorry.

agricolab commented 3 years ago

So, I updated Multitaper and build it. When i run test Multitaper from the Atom / Juno REPL, it errors out. With different errors, though (see below). When i run test either with julia test/runtest.jl or julia -e 'import Pkg; Pkg.build(); Pkg.test("Multitaper"; coverage=true)' from bash, it runs fine. So i tried import Pkg; Pkg.build(); Pkg.test("Multitaper"; coverage=true) from the REPL, but it errors out. There seems to be an issue with Atom/Juno.

Consider Travis is fine, the errors the REPL throws all boil down to float imprecision, i'd say it's my machine, your package is not to blame, and tests do pass. 

Spectrum analysis with gaps: Test Failed at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:34
  Expression: ((out[1])[1]).S[end - 5:end] ≈ [0.14565610885883232, 0.13580424778417852, 0.1293182864247158, 0.1395702616312291, 0.1664040092175493, 0.09202992618124456]
   Evaluated: [0.14543356846811006, 0.13562233447778846, 0.12902790267062653, 0.1390735544250517, 0.16571108838230617, 0.09180531734079359] ≈ [0.14565610885883232, 0.13580424778417852, 0.1293182864247158, 0.1395702616312291, 0.1664040092175493, 0.09202992618124456]
Stacktrace:
 [1] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:34
 [2] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
 [3] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:34
Spectrum analysis with gaps: Test Failed at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:41
  Expression: (out1[2])[end - 5:end] ≈ [17.444104148892166, 17.408259466237617, 17.382026447855413, 17.422494161582264, 17.506998428080035, 17.170675010806065]
   Evaluated: [17.444213771865872, 17.40836462891643, 17.395835313061312, 17.422698686063306, 17.50720418110945, 17.170799445545875] ≈ [17.444104148892166, 17.408259466237617, 17.382026447855413, 17.422494161582264, 17.506998428080035, 17.170675010806065]
Stacktrace:
 [1] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:41
 [2] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
 [3] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:34
Spectrum analysis with gaps: Test Failed at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:45
  Expression: (out1[1]).Fpval[end - 5:end] ≈ [0.16807947246930965, 0.09822111646921738, 0.0341711343274661, 0.4933223598055142, 0.9312676978366197, 0.7307073177440282]
   Evaluated: [0.16807735168842242, 0.09821956726975023, 0.03406832345614996, 0.49331774315244115, 0.9312668180688901, 0.7307054384542364] ≈ [0.16807947246930965, 0.09822111646921738, 0.0341711343274661, 0.4933223598055142, 0.9312676978366197, 0.7307073177440282]
Stacktrace:
 [1] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:45
 [2] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
 [3] top-level scope at /home/user/.julia/packages/Multitaper/qFw26/test/mdmwps_test.jl:34
Test Summary:                 | Pass  Fail  Total
mdmwps multitaper             |    6     3      9
  Slepians with gaps          |    1            1
  Spectrum analysis with gaps |    2     3      5
  Coherence with gaps         |    1            1
  Generalized Slepians        |    2            2
anowacki commented 3 years ago

If this degree of variation (up to 0.0007 per element) is to be expected between machines/versions, then it would be useful to add an appropriate value for rtol or atol to the approximate equality test so it still passes. Or if it isn't expected, then a fix should be found. (These differences are around 0.5% which doesn't seem totally insignificant and well above machine epsilon.)

   Evaluated:
[0.14543356846811006, 0.13562233447778846, 0.12902790267062653, 0.1390735544250517, 0.16571108838230617, 0.09180531734079359] ≈
[0.14565610885883232, 0.13580424778417852, 0.1293182864247158,  0.1395702616312291, 0.1664040092175493,  0.09202992618124456]
agricolab commented 3 years ago

@anowacki , you are probably right, although i hesitate to make this a blocker for publication, especially as it seems this is only my machine. Could you replicate the bug? I created an issue in the repo, in case we want to discuss this more https://bitbucket.org/clhaley/multitaper.jl/issues/33/floating-point-imprecision-with-tests-in

anowacki commented 3 years ago

@agricolab No, I agree with you and I can't reproduce it. I too don't think it should be a blocker—there will be bugs and issues with all software, of course.

agricolab commented 3 years ago

I browsed through the examples and explored a little bit more of the toolbox, and can now faithfully recommend for publication. Great job, and looking forward to see it being used!

Thanks for the offer to be mentioned in the acknowledgments, and the kind words, even if my contributions have certainly not been indispensible.

agricolab commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

anowacki commented 3 years ago

@VivianePons I can also say I'm happy with the paper now and have ticked all my boxes above. A very nice package, @lootie, and I hope you're happy with how it stands now. I would recommend registering the package once you have sorted out the compat bounds.

lootie commented 3 years ago

@anowacki @agricolab Thank you both for the comments regarding the interesting behavior in Atom/Juno. I am unable to replicate this either, but I'm inclined to increase the tolerance for the mdmultispec test that failed, for the time being. I'll update the bitbucket issue once I make the change.

Thank you for your effort in checking the package over in detail. I'm very pleased with the results of this review.

VivianePons commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: