search

openjournals / joss-reviews

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

[REVIEW]: musclesyneRgies: factorization of electromyographic data in R with sensible defaults #4439

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@alesantuz<!--end-author-handle-- (Alessandro Santuz) Repository: https://github.com/alesantuz/musclesyneRgies Branch with paper.md (empty if default branch): master Version: v1.2.3 Editor: !--editor-->@fabian-s<!--end-editor-- Reviewers: @SimonDanner, @vbaliga Archive: 10.5281/zenodo.6767313

Status

status

Status badge code:

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

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

@SimonDanner & @vbaliga, 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 @fabian-s 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 @SimonDanner

📝 Checklist for @vbaliga

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.08 s (601.9 files/s, 90696.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               26            280            947           1590
HTML                             2            203             10           1387
Markdown                        10            282              0            954
Rmd                              7            173            538            494
SVG                              2              0              0            321
TeX                              1             20              0            269
YAML                             2             11              4             51
-------------------------------------------------------------------------------
SUM:                            50            969           1499           5066
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1584

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

OK DOIs

- 10.1016/j.conb.2009.09.002 is OK
- 10.1152/jn.00625.2019 is OK
- 10.3389/fncom.2013.00051 is OK
- 10.1038/44565 is OK
- 10.1038/5721 is OK
- 10.1152/jn.00052.2018 is OK
- 10.1038/s41598-020-65257-w is OK
- 10.1162/NECO_a_00576 is OK
- 10.1016/j.medengphy.2018.04.003 is OK
- 10.3389/fnhum.2014.00335 is OK
- 10.1142/S0129065717500071 is OK
- 10.1155/2018/5852307 is OK
- 10.1152/jn.00275.2014 is OK
- 10.1016/0167-2789(88)90081-4 is OK
- 10.1152/jn.00360.2020 is OK
- 10.1061/TACEAT.0006518 is OK
- 10.1016/0167-2789(93)90009-P is OK
- 10.1016/j.isci.2019.100796 is OK
- 10.1016/j.gaitpost.2005.11.004 is OK

MISSING DOIs

- 10.1016/0006-8993(69)90278-9 may be a valid DOI for title: The co-ordination and regulation of movements

INVALID DOIs

- None
editorialbot commented 2 years ago

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

fabian-s commented 2 years ago

👋🏼 @alesantuz, @SimonDanner, @vbaliga this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#REVIEW_NUMBER so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@fabian-s) if you have any questions/concerns.

fabian-s commented 2 years ago

@alesantuz, the paper (~1600 words) is currently rather long for JOSS, we aim for <1000 words.

You could consider moving the "typical workflow" section into a vignette for your package and replacing that section in your article with a more high-level description of the functionality, for example.

EDIT: should have been more clear: "<1000 words" is not a hard, mandatory thing, but we would prefer if you could shorten your paper by some amount.

alesantuz commented 2 years ago

@alesantuz, the paper (~1600 words) is currently rather long for JOSS, we aim for <1000 words.

You could consider moving the "typical workflow" section into a vignette for your package and replacing that section in your article with a more high-level description of the functionality, for example.

EDIT: should have been more clear: "<1000 words" is not a hard, mandatory thing, but we would prefer if you could shorten your paper by some amount.

@fabian-s I can certainly do that, since "workflow" is already a vignette which is anyway more detailed than the text in the paper. I will cut that section down to the bone, perhaps leaving only the bullet list and the compact code example (lines 47 to 57). I'd leave the figure though, if that's ok.

fabian-s commented 2 years ago

"workflow" is already a vignette which is anyway more detailed than the text in the paper.

cool, so that won't be much trouble anyways.

I will cut that section down to the bone, perhaps leaving only the bullet list and the compact code example (lines 47 to 57). I'd leave the figure though, if that's ok.

perfect!

alesantuz commented 2 years ago

@fabian-s wonderful, thank you. I amended the previous version.

alesantuz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

alesantuz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

SimonDanner commented 2 years ago

Review checklist for @SimonDanner

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

vbaliga commented 2 years ago

Review checklist for @vbaliga

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

COMMENTS:

All in all, this is a nicely-written submission and I hope these comments are helpful. Feel free to let me know if I misunderstood anything and/or can help clarify my feedback.

Cheers, Vikram 🐢

alesantuz commented 2 years ago

Thank you @SimonDanner and @vbaliga! I will certainly attempt to address all your points and thanks for the issue about the README.

alesantuz commented 2 years ago

@vbaliga, @fabian-s here my replies. Thanks again Vikram for the deep analysis! The package underwent some solid improvements.

  • The JOSS paper submission does a nice job of defining what a "synergy" is in the context of muscle physiology, but the README does not provide an indication of what this word means. Given that a user's first point of entry into this package may sidestep the JOSS paper, I'll suggest that the first section of the package README should also give a definition for "synergy". This of course need not be long -- I am hoping that a 1-2 sentence description could suffice.

A definition has been added to the README with this commit.

  • "Motor primitive" might also be worth briefly defining.

Motor primitives and also motor modules have been defined with this commit.

  • The classify_kmeans() function is quite valuable to the end-goal of the package and the function itself is well-written. That said, I do wish there were more details and advice given to users. As I'm sure the author knows, kmeans clustering can be stochastic, and I think it would be valuable to guide end-users through exactly what aspects of the clustering might be more variable if-rerun. It's great that there's arguments like MSE_lim and inspect to help control the behavior of the function, but I do think there should be more detail surrounding these (perhaps fleshed out in a vignette?).

More detail has been added to the documentation with this commit.

  • I'll also note that there doesn't seem to be a formal testthat test for classify_kmeans(). Although the function does appear in test-plots.R, the behavior of the kmeans function itself doesn't seem to be under scrutiny here. I fully understand that the end result of classify_kmeans() might be hard to write a test for, but given the importance of the function, I wonder if some internal aspects can be tested

A test, perhaps naif, for the classify_kmeans function has been added with this commit. Other tests added thanks to the suggestions by @fabian-s.

  • The Help files are generally well-written and helpful. I do wish there were a little more detail to the entries for the HFD() and Hurst() functions. What do these values mean? Long details of the histories of each metric are of course not necessary, but it would be good to give users a better conceptual understanding of what is being computed and what it signifies.

The two functions are now deeper explained in the relevant documentation, please see this commit.

  • I have made an issue (Readme example alesantuz/musclesyneRgies#68) to note that the first block of package code in the README does not work. This is potentially an intentional move by the author (given the text immediately above the block), but as a new user of the package, I'd expect that the first example code showcases worked example(s) rather than pseudocode.

This has now been solved with this commit by linking to a test data set that was actually used in the earliest versions of this package (pre-CRAN when data set space requirements did not exist).

  • There are two LICENSE files, one of which is a standard MIT license (LICENSE.md) and one that simply notes the author's name and the date (LICENSE). Is the latter necessary? The appearance of two files is why I did not check off the corresponding item in the checklist above, but I'm happy to check it off if having both files presents no issues.

Since I am using the repo for the development of the R-package, I am afraid the plain LICENSE file must contain only year and copyright holder's name and CRAN did make me change that. Thus the double version, which is not problematic for either CRAN or GitHub.

All the above changes are now published as a new development release, v1.1.3.9006.

Once more thanks for the commitment to conduct this round of reviews!

fabian-s commented 2 years ago

@vbaliga thanks a lot for your careful review and constructive comments - if you feel the issues you pointed out have been addressed satisfactorily, please check off the respective items in your checklist. Re the LICENSE/LICENSE.md issue: as @alesantuz explained, this is standard operating procedure for CRAN packages.

@alesantuz I have some additional suggestions that should hopefully be easy to deal with if you choose to do so, see repo.

alesantuz commented 2 years ago

Thank you @fabian-s, I will dive into your great suggestions right away!

vbaliga commented 2 years ago

Hi @alesantuz and @fabian-s

I'm quite happy with the edits that Ale made regarding my comments, and have checked off the corresponding items in the checklist. It's great to see this type of work being published open-source and I think it will be a fine addition to JOSS. Please feel free to let me know if you need anything else from me

Best regards, Vikram 🐢

alesantuz commented 2 years ago

Thank you @vbaliga! @fabian-s is anything needed on my side at this stage?

fabian-s commented 2 years ago

Not really, no, I may have misunderstood your " dive into your great suggestions" and "all the rest is going to be addressed soon" remarks which is why I did not move forward with this yet.
I thought your remarks meant that you were about to adress all of the points in alesantuz/musclesyneRgies#69 and wanted to wait until you had done so, but now I understand you meant this more as "I'll add some unit tests, and eventually maybe deal with the rest".

If you don't intend to add these additional improvements right now, we can move towards publication here. If so, please:

I can then move forward with recommending to accept the submission.

alesantuz commented 2 years ago

Not really, no, I may have misunderstood your " dive into your great suggestions" and "all the rest is going to be addressed soon" remarks which is why I did not move forward with this yet. I thought your remarks meant that you were about to adress all of the points in alesantuz/musclesyneRgies#69 and wanted to wait until you had done so, but now I understand you meant this more as "I'll add some unit tests, and eventually maybe deal with the rest".

I added the most relevant unit tests while leaving open some that in my opinion are not quite worth of testing (e.g. I suppose that all those if(interactive()) don't actually need unit testing and are there only because of the CRAN tests, please correct me if I am wrong). All the rest has been fixed in version 1.1.3.9006 (e.g. avoid using 1:ncol() or similar, write TRUE or FALSE in full, etc.).

  • [x] Make a tagged release of your software, and list the version tag of the archived version here.
  • [x] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
  • [x] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
  • [x] Please list the DOI of the archived version here.

Version 1.2.0 is archived on GitHub here and on Zenodo here with DOI 10.5281/zenodo.6670567.

fabian-s commented 2 years ago

wonderful, thanks! I missed those other commits, sorry.

fabian-s commented 2 years ago

@editorialbot generate pdf

fabian-s commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.conb.2009.09.002 is OK
- 10.1152/jn.00625.2019 is OK
- 10.3389/fncom.2013.00051 is OK
- 10.1038/44565 is OK
- 10.1038/5721 is OK
- 10.1152/jn.00052.2018 is OK
- 10.1038/s41598-020-65257-w is OK
- 10.1162/NECO_a_00576 is OK
- 10.1016/j.medengphy.2018.04.003 is OK
- 10.3389/fnhum.2014.00335 is OK
- 10.1142/S0129065717500071 is OK
- 10.1155/2018/5852307 is OK
- 10.1152/jn.00275.2014 is OK
- 10.1016/0167-2789(88)90081-4 is OK
- 10.1152/jn.00360.2020 is OK
- 10.1061/TACEAT.0006518 is OK
- 10.1016/0167-2789(93)90009-P is OK
- 10.1016/j.isci.2019.100796 is OK
- 10.1016/j.gaitpost.2005.11.004 is OK

MISSING DOIs

- 10.1016/0006-8993(69)90278-9 may be a valid DOI for title: The co-ordination and regulation of movements

INVALID DOIs

- None
editorialbot commented 2 years ago

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

fabian-s commented 2 years ago

@alesantuz spell check was mostly fine too, maybe consider using "center" instead of "centre" in l. 44, since you use american spelling everywhere else?

Also please add that missing DOI editorialbot suggested, seems to be the right one for that old reference: image

alesantuz commented 2 years ago

@fabian-s

@alesantuz spell check was mostly fine too, maybe consider using "center" instead of "centre" in l. 44, since you use american spelling everywhere else?

Thank you, I also corrected a couple of other instances where there was a mixup between Brit and American English. Should I update Zenodo and/or create a new version after the spell check?

About the DOI and as mentioned in the pre-review:

the suggested DOI to replace the missing one does not refer to the correct publication and links instead to the following, which is just a "Book notice" from the journal Brain Research

I still could not find a DOI that actually fits this book.

alesantuz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

fabian-s commented 2 years ago

Thank you, I also corrected a couple of other instances where there was a mixup between Brit and American English. Should I update Zenodo and/or create a new version after the spell check?

great, yes, please update zenodo. sorry, I should have checked that before asking you to create the zenodo deposit...

the suggested DOI to replace the missing one does not refer to the correct publication and links instead to the following, which is just a "Book notice" from the journal Brain Research

sure, sorry for forgetting we already covered this.

alesantuz commented 2 years ago

No worries @fabian-s. Version 1.2.1 is archived on GitHub here and on Zenodo here with DOI 10.5281/zenodo.6671216.

fabian-s commented 2 years ago

@editorialbot set 10.5281/zenodo.6671216 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6671216

fabian-s commented 2 years ago

@editorialbot set v1.2.1 as version

editorialbot commented 2 years ago

Done! version is now v1.2.1

fabian-s commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.conb.2009.09.002 is OK
- 10.1152/jn.00625.2019 is OK
- 10.3389/fncom.2013.00051 is OK
- 10.1038/44565 is OK
- 10.1038/5721 is OK
- 10.1152/jn.00052.2018 is OK
- 10.1038/s41598-020-65257-w is OK
- 10.1162/NECO_a_00576 is OK
- 10.1016/j.medengphy.2018.04.003 is OK
- 10.3389/fnhum.2014.00335 is OK
- 10.1142/S0129065717500071 is OK
- 10.1155/2018/5852307 is OK
- 10.1152/jn.00275.2014 is OK
- 10.1016/0167-2789(88)90081-4 is OK
- 10.1152/jn.00360.2020 is OK
- 10.1061/TACEAT.0006518 is OK
- 10.1016/0167-2789(93)90009-P is OK
- 10.1016/j.isci.2019.100796 is OK
- 10.1016/j.gaitpost.2005.11.004 is OK

MISSING DOIs

- 10.1016/0006-8993(69)90278-9 may be a valid DOI for title: The co-ordination and regulation of movements

INVALID DOIs

- None
fabian-s commented 2 years ago

@alesantuz thanks for taking care of this so quickly!

fabian-s commented 2 years ago

note to EiCs: "missing" DOI suggested above is wrong, the paper is from 1967 and does not have a DOI.

editorialbot commented 2 years ago

:warning: Error prepararing paper acceptance. The generated XML metadata file is invalid. 

Element isbn: [facet 'minLength'] The value has a length of '9'; this underruns the allowed minimum length of '10'.
Element isbn: [facet 'minLength'] The value has a length of '7'; this underruns the allowed minimum length of '10'.
alesantuz commented 2 years ago

@fabian-s I tried to understand where that error comes from and there might have been too many commas in the .bib file. I made a new version, 1.2.2, which is archived on GitHub here and on Zenodo here with DOI 10.5281/zenodo.6673110.

alesantuz commented 2 years ago

@editorialbot set 10.5281/zenodo.6673110 as archive

editorialbot commented 2 years ago

I'm sorry @alesantuz, I'm afraid I can't do that. That's something only editors are allowed to do.

fabian-s commented 2 years ago

@editorialbot set 10.5281/zenodo.6673110 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6673110