search

openjournals / joss-reviews

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

[REVIEW]: libfmp: A Python Package for Fundamentals of Music Processing #3326

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @fzalkow (Frank Zalkow) Repository: https://github.com/meinardmueller/libfmp Version: v1.2.1 Editor: @arfon Reviewer: @brunaw, @expectopatronum Archive: 10.5281/zenodo.5113869

: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/47b595f5a3d0259d9ae5574e0d4a09b7"><img src="https://joss.theoj.org/papers/47b595f5a3d0259d9ae5574e0d4a09b7/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/47b595f5a3d0259d9ae5574e0d4a09b7/status.svg)](https://joss.theoj.org/papers/47b595f5a3d0259d9ae5574e0d4a09b7)

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

@brunaw & @expectopatronum, 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 @arfon 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 ✨

Review checklist for @brunaw

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @expectopatronum

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @brunaw, @expectopatronum 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 3 years ago

PDF failed to compile for issue #3326 with the following error: 

 Can't find any papers to compile :-(
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.36 s (423.6 files/s, 164540.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            59           8855            174          21375
JavaScript                      14           2404           2467           9203
Python                          55           1880           3643           4641
SVG                              1              0              0           2671
CSS                              4            181             33            726
reStructuredText                13             47            181             66
DOS Batch                        1              8              1             26
Markdown                         2             18              0             22
make                             1              4              7              9
YAML                             1              0              2              6
-------------------------------------------------------------------------------
SUM:                           151          13397           6508          38745
-------------------------------------------------------------------------------

Statistical information for the repository '9f562e921fc63288d9416627' was
gathered on 2021/06/01.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Frank Zalkow                     6         16413           1871           65.33
Meinard Müller                   3          9700              4           34.67

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Frank Zalkow              16405          100.0          0.6               15.56
Meinard Müller             7833           80.8          0.1               10.29
arfon commented 3 years ago

@brunaw, @expectopatronum – This is the review thread for the paper. All of our communications will happen here from now on.

Please read the "Reviewer instructions & questions" in the first comment above.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also 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 https://github.com/openjournals/joss-reviews/issues/3326 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 the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.

arfon commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
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:

whedon commented 3 years ago

:wave: @brunaw, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @expectopatronum, please update us on how your review is going (this is an automated reminder).

brunaw commented 3 years ago

@whedon I will start the review next week

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
expectopatronum commented 3 years ago

This is a more general question about how to understand "Substantial scholarly effort". If I would follow the list here, I would have to say no because:

"positive" answers:

"negative" answers (leading to saying no):

The two "positive" answers might outweigh some of the negatives. I don't think that the age of the software / the number of commits / number of authors should have much to say, as I would say that also one person could create a very useful software package, and 10 people might not; and additionally, the age of the git repository and the number of commits might not reflect the actual development effort as many software packages start in a private repository.

This leaves me with the last question (according to my reordered list), which is actually important. As mentioned above "libfmp" is only mentioned in papers by the same group. Since libfmp has only been available via pip for a couple of months, and was not promoted before (I think), this seems reasonable. So I also checked the papers citing the FMP notebooks paper which I would expect people to cite if they were using the code and it looks like the FMP notebooks paper is exclusively cited by work from the same group.

Maybe @arfon can clarify how this question should be answered given the information I provided. Thanks!

expectopatronum commented 3 years ago

I created an issue for almost all the things I noticed. There are two other things that I am not sure about and might be interesting to discuss with the other reviewer or the editor:

Would be interesting to hear other people's opinion about those two things.

Best regards Verena

fzalkow commented 3 years ago

Many thanks for the thorough review process. I just want to clarify two of the mentioned points.

Age of software: The development of libfmp went hand in hand with developing the FMP notebooks, which have been developed internally before releasing libfmp on GitHub. This development process preceded the first GitHub release by a few years. Furthermore, some of the algorithms are based on previously unpublished internal MATLAB implementations, which predate FMP by several years.

Variable names: The deviations of the variable names compared to librosa is intentional. An essential aspect of the FMP notebooks and libfmp is to establish a close connection to the FMP book. This connection enables students to study fundamental MIR concepts in-depth by reading a textbook along with implementations of the concepts explained in the book. Thus, the variable names reflect the mathematical notation used in the book.

Best,

Frank

expectopatronum commented 3 years ago

Age of software ...

That's what I assumed! (and also doesn't have an impact on my review) The first comment was more of a question for the editor, since it is not clear to me what is expected from me for answering the "Substantial scholarly effort" question, although I am inclined to answer it with yes.

Variable names ...

Thanks for the clarification, makes sense!

If you can take care of the minor issues I raised here I can tick all the missing boxes and can finish my review.

arfon commented 3 years ago

Maybe @arfon can clarify how this question should be answered given the information I provided. Thanks!

Thanks for raising these questions about scholarly effort @expectopatronum – the 'signals' we suggest are just pointers you might be able to use to guide your thinking. We also say that the work should represent three or more months of effort which is sound like it does?

After reading the README in a little more detail myself I did have one question for the authors – could you clarify @fzalkow if this is software designed for addressing research challenges or is a teaching tool? JOSS doesn't publish software designed for teaching (we encourage authors to submit to our sister journal JOSE for submissions that fall into this category).

fzalkow commented 3 years ago

Dear arfon,

Many thanks for your question.

Indeed libfmp covers both teaching and research aspects. This combination is due to the origin of libfmp. The starting point is Müller's FMP book, where fundamental music information retrieval (MIR) algorithms are presented in a didactically prepared textbook. These algorithms summarize the essence from the research of many years. Then, we developed the FMP notebooks, which expand and complement the FMP book. Here, the aim is more on the teaching side. During the development of the FMP notebooks, many research-related algorithms have been collected in the libfmp package, which we now made available independent of the FMP notebooks. Thus, compared to the FMP notebooks, the research aspect is more dominant in libfmp. Due to this development, we consider libfmp an ideal tool to transition from studying (using the FMP notebooks and the FMP book) to research. The package is intended for research purposes and contains (similar to librosa) many implementations of core algorithms and advanced methods for MIR.

Concerning three or more months of effort: This holds undoubtedly. The development of libfmp started a few years ago. Considering the origins in the FMP book, it even can be traced back more than a decade.

Best,

Frank

arfon commented 3 years ago

The package is intended for research purposes and contains (similar to librosa) many implementations of core algorithms and advanced methods for MIR.

Got it. Thanks for the additional context and background.

@brunaw – do you think you might be able to complete your review soon?

brunaw commented 3 years ago

Dear @arfon,

I will complete my review next week, my apologies for the delay. Is that okay?

brunaw commented 3 years ago

@expectopatronum Just my two cents of the previous discussion. It's a little strange to me that the FMP notebooks paper isn't cited much because, at least from my experience, the notebooks are widely known in the ISMIR community. I know a few people who have used it in their research so they might have missed it when writing their references. This also leads to the age question, the first time I got to know the FMP notebooks was about ~3 years ago, and they were already very well developed/organized by them, so the software is certainly older than 6 months. I hope this helps with something.

arfon commented 3 years ago

I will complete my review next week, my apologies for the delay. Is that okay?

Absolutely! Thanks for the update.

brunaw commented 3 years ago

@arfon I am unable to change/click in my checklist items, could you please help me fix it?

danielskatz commented 3 years ago

@whedon re-invite @brunaw as reviewer

whedon commented 3 years ago

The reviewer already has a pending invite.

@brunaw please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

brunaw commented 3 years ago

@danielskatz My invitation has expired

danielskatz commented 3 years ago

@whedon re-invite @brunaw as reviewer

If this doesn't work, is there a chance you have multiple github accounts and the one with the invitation isn't the one your are logged into?

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@brunaw please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 3 years ago

This may have just been very bad timing

brunaw commented 3 years ago

@danielskatz I think it was just that I didn't do anything about the first invitation and it didn't let you re-invite me even thought the invitation was already expired

Thank you! and apologies for the trouble

brunaw commented 3 years ago

Dear @fzalkow,

Here are my comments/suggestions about the submission,

Reads a CSV file

And it could be something more complete like

Reads a CSV file in table format and creates a pd.DataFrame from it, with observations in the rows and variables in the columns

Just a quick compliment, I think this was one of the best submissions I've reviewed so far. The paper was well written and I was able to check all boxes right away. The issues pointed by the first reviewer were well addressed, so I would like to congratulate the authors for the good work.

meinardmueller commented 3 years ago

Dear @brunaw,

Thanks for your suggestions and comments. As for your first point, we followed your suggestion by adding one more headline, which better structures the paper. As for the second point, we have the following suggestion. Most of the libfmp-functions are introduced step-by-step and explained in the FMP Notebooks; in the header of the libfmp-functions, we provided a link to the respective FMP notebook (provided as static html-version on our website). There are only a few libfmp-functions that do not have a companion in the FMP notebooks (e.g., the one you mentioned). For those functions, we would add some more details in the header as suggested by you. Doing this , we would require some weeks (also due to holidays). Does this sound ok for you?

fzalkow commented 3 years ago

Dear @brunaw,

I just implemented Meinard's suggestion regarding your second point (meinardmueller/libfmp@f9e8895d2b529ee6f813afe7d3ecfa08517d5fb4, meinardmueller/libfmp@57905e09555259d5b8b9688934a6695f638d06ee). I am currently on holiday – so sorry for our somewhat asynchronous communication.

Meinard modified the markdown. So, @whedon generate pdf from branch paper.

arfon commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
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:

brunaw commented 3 years ago

@meinardmueller @fzalkow Yes, sounds good to me. I just saw the new changes in the paper and the function descriptions and it all looks good, thank you

arfon commented 3 years ago

@meinardmueller @fzalkow Yes, sounds good to me. I just saw the new changes in the paper and the function descriptions and it all looks good, thank you

:zap: great! I think based on the feedback of both of our reviewers here we're in good shape to move forward.

@fzalkow - Once you've made all of the changes you plan to could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:

I can then move forward with accepting the submission.

fzalkow commented 3 years ago

Dear @arfon, we made a new release (latest: v1.2.1) and a Zenodo archive (DOI: 10.5281/zenodo.5113869). Please let us know if everything looks right. Many thanks.

arfon commented 3 years ago

@whedon set 10.5281/zenodo.5113869 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5113869 is the archive.

arfon commented 3 years ago

@whedon set v1.2.1 as version

whedon commented 3 years ago

OK. v1.2.1 is the version.

arfon commented 3 years ago

@whedon recommend-accept from branch paper

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago

PDF failed to compile for issue #3326 with the following error: 

 /app/vendor/bundle/ruby/2.6.0/gems/bibtex-ruby-6.0.0/lib/bibtex/bibliography.rb:50:in `read': No such file or directory @ rb_sysopen - 5c6867af5266b997a2e1f6c4/paper/["references.bib"] (Errno::ENOENT)
    from /app/vendor/bundle/ruby/2.6.0/gems/bibtex-ruby-6.0.0/lib/bibtex/bibliography.rb:50:in `open'
    from /app/vendor/bundle/ruby/2.6.0/gems/bibtex-ruby-6.0.0/lib/bibtex/utilities.rb:25:in `open'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/lib/whedon/bibtex_parser.rb:38:in `generate_citations'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/lib/whedon/compilers.rb:253:in `crossref_from_markdown'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/lib/whedon/compilers.rb:21:in `generate_crossref'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/lib/whedon/processor.rb:100:in `compile'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/bin/whedon:88:in `compile'
    from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in `run'
    from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in `invoke_command'
    from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in `dispatch'
    from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:in `start'
    from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-b63fc70cc085/bin/whedon:131:in `<top (required)>'
    from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `load'
    from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `<main>'
arfon commented 3 years ago

@fzalkow – could you please merge this PR which fixes your paper: https://github.com/meinardmueller/libfmp/pull/5

fzalkow commented 3 years ago

@fzalkow – could you please merge this PR which fixes your paper: meinardmueller/libfmp#5

Done!

arfon commented 3 years ago

@whedon recommend-accept from branch paper

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

OK DOIs

- 10.25080/Majora-7b98e3ed-003 is OK
- 10.1109/MSP.2018.2876190 is OK
- 10.5281/zenodo.1415016 is OK
- 10.1145/2964284.2973795 is OK
- 10.1145/1631272.1631459 is OK
- 10.5281/zenodo.1417145 is OK
- 10.5281/zenodo.3527872 is OK
- 10.1109/TASL.2010.2096216 is OK
- 10.1109/ICASSP.2010.5495219 is OK
- 10.1109/TASL.2012.2227732 is OK
- 10.5281/zenodo.1416024 is OK
- 10.5281/zenodo.1416800 is OK
- 10.5281/zenodo.1415226 is OK
- 10.1109/ICASSP.2012.6287834 is OK

MISSING DOIs

- 10.1007/978-3-319-21945-5 may be a valid DOI for title: Fundamentals of Music Processing

INVALID DOIs

- None