openjournals / joss-reviews

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

[REVIEW]: onsetsync: An R Package for Onset Synchrony Analysis #5395

Closed editorialbot closed 9 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@tuomaseerola<!--end-author-handle-- (Tuomas Eerola) Repository: https://github.com/tuomaseerola/onsetsync Branch with paper.md (empty if default branch): master Version: 0.5.1 Editor: !--editor-->@faroit<!--end-editor-- Reviewers: @pmcharrison, @mrocamora Archive: 10.5281/zenodo.10050346

Status

status

Status badge code:

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

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

@pmcharrison & @mrocamora, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @mrocamora

📝 Checklist for @pmcharrison

tuomaseerola commented 1 year ago

I agree. The minimal edit is to adjust the size of the labels [Done] and explicit sign (+ and -) [DONE] and I also agree that it is not optimal to have visualisation and tabular information combined, but this was a user request. I can do a new switch that plots these as marginal lines and leaves the unlabelled onset times as the default, but for now the visibility improvement should be sufficient.

tuomaseerola commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

pmcharrison commented 1 year ago

Thanks @tuomaseerola, let me know if you want me to look at the updated figure (the latest PDF looks the same to me as before)

image
tuomaseerola commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

tuomaseerola commented 1 year ago

@editorialbot set R1 as branch

editorialbot commented 1 year ago

Done! branch is now R1

tuomaseerola commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

tuomaseerola commented 1 year ago

@pmcharrison, I had to direct the editorialbot to work in R1 branch before it compiled the updated version.

pmcharrison commented 1 year ago

Can confirm that new version looks great!

pmcharrison commented 1 year ago

@faroit I'm happy to go ahead

faroit commented 1 year ago

@pmcharrison thanks for the update. happy to see that this submission is on track for acceptance.

faroit commented 1 year ago

@mrocamora can you give us and update on the last unchecked item in your review?

State of the field: Do the authors describe how this software compares to other commonly-used packages?

mrocamora commented 1 year ago

Yes, I think this is solved. There is no need to mention SyncPy and there are several examples of tools for onset extraction.

mrocamora commented 1 year ago

Y confirm that the new version looks great too! @faroit I am happy to go ahead.

BTW please note that lines 74 to 89 of the pdf (the R CMD build output) should be removed.

faroit commented 1 year ago

@tuomaseerola happy to continue with the last steps now.

BTW please note that lines 74 to 89 of the pdf (the R CMD build output) should be removed.

can you modify this, please and let me know when the code is up-to-date?

tuomaseerola commented 1 year ago

@tuomaseerola happy to continue with the last steps now.

BTW please note that lines 74 to 89 of the pdf (the R CMD build output) should be removed.

can you modify this, please and let me know when the code is up-to-date?

Yes, @faroit, the installation message is now "quiet" and the actual installation refers to the main repo, not to the R1 branch, which means I need to merge R1 with the main once this review process is complete. For changes, see https://github.com/tuomaseerola/onsetsync/commit/549b9fe44d6ba9184ce65159ff3fed1549925d16

pmcharrison commented 1 year ago

@tuomaseerola I had a student try to run some analyses using the package and they struggled to work out what kind of format is required when providing one's own dataset. There is some information on this in the paper but I think it would be helpful to document it in the package itself too, ideally for all functions that ingest such datasets (roxygen has functionality for inheriting documentation so you don't have to copy and paste each time). Ideally this documentation would explain which columns are mandatory and which are optional. I also found the definition of Isochronous.SD.Time a bit confusing and wondered whether it could be explained a bit clearer, e.g. through the use of an example?

faroit commented 1 year ago

@pmcharrison i understand that this would be useful but I feel as this time this would be out of scope for the JOSS submission as it's a about to get accepted soon. Cant you just create a GitHub issue?

pmcharrison commented 1 year ago

I have filed an issue too now. I do recommend it be addressed before publication though, otherwise anyone who sees the package published and tries to run it on their own data will likely suffer from similar issues.

tuomaseerola commented 1 year ago

I do recognise the issue myself, the existing examples are way above the "minimal required" information and explaining the required representations (columns of data) would be helpful. I have not thought through whether every function then would have to be checked and signposted for the specific requirements, it probably needs input check routines and documentation for each function, and turns easily into changing the the input to S3 classes but if the minimal change would be to add documentation to the package using a vignette (e.g. "input-representation"), this could be an easy task.

pmcharrison commented 1 year ago

Hi @tuomaseerola, the vignette would indeed be a nice solution and you should do that if you think it's most efficient. The other (near-?) minimal solution would be to go to plot_by_beat.R, and update this line:

#' @param df data frame to be processed

to specify complete information about the required columns, then in every other function that uses such a dataset you delete that line and instead write

#' @inheritParams plot_by_beat
faroit commented 1 year ago

@pmcharrison can you please link the issue back here and discuss the solution in that issue? Once that has been closed we should move on with the next steps. Thanks!

pmcharrison commented 1 year ago

Here is the issue: https://github.com/tuomaseerola/onsetsync/issues/12

faroit commented 1 year ago

@pmcharrison the issue seems to be closed, can we proceed with the review?

pmcharrison commented 1 year ago

@faroit the review is complete on my end, I gave my approval a while ago

faroit commented 1 year ago

@faroit the review is complete on my end, I gave my approval a while ago

@pmcharrison i was referring here to your previous message:

I have filed an issue too now. I do recommend it be addressed before publication though,

Thus, I thought the closing of the issue was required to complete the review.

In any case, glad to see that we can move on

pmcharrison commented 1 year ago

I see, yes the issue has been nicely addressed now.

-- Sent from my mobile


From: Fabian-Robert Stöter @.> Sent: Wednesday, October 11, 2023 6:19:55 AM To: openjournals/joss-reviews @.> Cc: Peter Harrison @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: onsetsync: An R Package for Onset Synchrony Analysis (Issue #5395)

@faroithttps://github.com/faroit the review is complete on my end, I gave my approval a while ago

@pmcharrisonhttps://github.com/pmcharrison i was referring here to your previous message:

I have filed an issue too now. I do recommend it be addressed before publication though,

Thus, I thought the closing of the issue was required to complete the review.

In any case, glad to see that we can move on

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5395#issuecomment-1756826384, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFNCKRRB4XXEE35VBN6PGPTX6YT7XAVCNFSM6AAAAAAXHKB5Y2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJWHAZDMMZYGQ. You are receiving this because you were mentioned.Message ID: @.***>

faroit commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

faroit commented 1 year ago

thanks @mrocamora and @pmcharrison for your reviews. We can now proceed with the post-review phase.

@tuomaseerola do you plan to merge the R1 branch into master before I start with the remaining steps?

tuomaseerola commented 1 year ago

@faroit Yes, I just merged R1 into the main. The html documentation needs a fix to avoid pointing to @R1 branch in installing, but I'll fix that but the otherwise this is functional in the main branch.

faroit commented 1 year ago

@editorialbot set master as branch

editorialbot commented 1 year ago

Done! branch is now master

faroit commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

faroit commented 1 year ago

@tuomaseerola i just did a proof read as the final step from my side. When that is done, can you please proceed with the post-review checklist that is posted above here?

tuomaseerola commented 1 year ago
tuomaseerola commented 1 year ago

@tuomaseerola i just did a proof read as the final step from my side. When that is done, can you please proceed with the post-review checklist that is posted above here?

@faroit Yes, all done.

faroit commented 1 year ago

@editorialbot set 10.5281/zenodo.10050346 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.10050346

faroit commented 1 year ago

@editorialbot set 0.5.1 as version

editorialbot commented 1 year ago

Done! version is now 0.5.1

faroit commented 1 year ago

@editorialbot generate pdf

faroit commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago

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

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1098/rsos.171520 is OK
- 10.17605/OSF.IO/SFXA2 is OK
- 10.17605/OSF.IO/KS325 is OK
- 10.1177/1029864919844809 is OK
- 10.18061/emr.v16i1.7555 is OK
- 10.18061/emr.v16i1.7555 is OK

MISSING DOIs

- 10.1162/comj_a_00002 may be a valid DOI for title: Investigations of between-hand synchronization in Magaloff’s Chopin
- 10.18061/1811/51203 may be a valid DOI for title: Inter-group entrainment in Afro-Brazilian Congado ritual
- 10.1109/icassp.2014.6854953 may be a valid DOI for title: Improved musical onset detection with convolutional neural networks
- 10.3389/fpsyg.2018.02232 may be a valid DOI for title: Analyzing multivariate dynamics using cross-recurrence quantification analysis (CRQA), diagonal-cross-recurrence profiles (dcrp), and multidimensional recurrence quantification analysis (MDRQA)– A tutorial in R
- 10.1177/0305735614555790 may be a valid DOI for title: Timing deviations in jazz performance: The relationships of selected musical variables on horizontal and vertical timing relations: A case study
- 10.1109/lsp.2020.3045915 may be a valid DOI for title: Adversarial unsupervised domain adaptation for harmonic-percussive source separation
- 10.1145/2660168.2660182 may be a valid DOI for title: Creating corpora for computational research in Arab-Andalusian music
- 10.1109/icassp.2016.7471640 may be a valid DOI for title: A generalized Bayesian model for tracking long metrical cycles in acoustic music signals
- 10.1098/rsif.2013.1125 may be a valid DOI for title: Optimal feedback correction in string quartet synchronization
- 10.3758/s13423-012-0371-2 may be a valid DOI for title: Sensorimotor synchronization: a review of recent research (2006–2012)
- 10.3758/bf03206433 may be a valid DOI for title: Sensorimotor synchronization: A review of the tapping literature
- 10.3389/fnins.2016.00285 may be a valid DOI for title: Both isochronous and non-isochronous metrical subdivision afford precise and stable ensemble entrainment: A corpus study of Malian jembe drumming
- 10.1016/j.humov.2008.02.016 may be a valid DOI for title: Sensorimotor synchronization with adaptively timed sequences

INVALID DOIs

- None
faroit commented 1 year ago

@tuomaseerola there are still a couple missing DOIs in the references ☝️

Please add them as available. No need to create a new version after doing changes to the paper

tuomaseerola commented 1 year ago

@faroit All added and DOIs double-checked with https://search.crossref.org.