openjournals / joss-reviews

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

[REVIEW]: Olaf: a lightweight, portable audio search system #5459

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@JorenSix<!--end-author-handle-- (Joren Six) Repository: https://github.com/JorenSix/Olaf Branch with paper.md (empty if default branch): Version: v1.1.0 Editor: !--editor-->@bmcfee<!--end-editor-- Reviewers: @liscio, @ebezzam Archive: 10.5281/zenodo.8093527

Status

status

Status badge code:

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

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

@liscio & @ebezzam, 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 @bmcfee 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 @liscio

📝 Checklist for @ebezzam

editorialbot commented 1 year 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 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (443.0 files/s, 342772.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                    22            490           2742          27735
C                               25           2256           2955          12901
Ruby                             7            250            105            901
SVG                              3              4              0            537
TeX                              1             47              0            322
make                             1             14             11            136
Arduino Sketch                   1             35              7             85
HTML                             1             13              0             60
Markdown                         1             22              0             43
JavaScript                       1             14              2             35
R                                1              4              0             34
YAML                             2              5              8             32
Dockerfile                       1              8             22             12
-------------------------------------------------------------------------------
SUM:                            67           3162           5852          42833
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1040

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

OK DOIs

- 10.1109/MMSP.2002.1203273 is OK
- 10.5281/zenodo.1416340 is OK
- 10.5281/zenodo.1417593 is OK
- 10.1109/TASL.2012.2234114 is OK
- 10.1007/978-3-319-73165-0_10 is OK
- 10.1007/s11265-005-4151-3 is OK
- 10.5281/zenodo.1414728 is OK
- 10.1121/1.400476 is OK
- 10.1121/1.404385 is OK
- 10.1109/CDMA47397.2020.00010 is OK
- 10.1145/3144749.3144755 is OK
- 10.1109/ICASSP39728.2021.9414337 is OK
- 10.5281/zenodo.1417008 is OK
- 10.5281/zenodo.1416190 is OK
- 10.1007/s12193-015-0196-1 is OK
- 10.48550/arXiv.2302.12258 is OK
- 10.5281/zenodo.1422385 is OK
- 10.5281/zenodo.1422385 is OK
- 10.5281/zenodo.1417973 is OK
- 10.5281/zenodo.4245544 is OK
- 10.21105/joss.04554 is OK
- 10.1007/s12193-015-0196-1 is OK
- 10.5281/zenodo.7343030 is OK
- 10.1007/978-3-031-18444-4_16 is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

liscio commented 1 year ago

Review checklist for @liscio

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

JorenSix commented 1 year ago

Just an update from here, I have fixed and closed the reported issues. They were mainly about fixing instructions and offering some proper error messages when things (tests) go wrong.

I have also updated the readme. The text grew over time and is now more properly structured. This is needed since there are instructions for running Olaf on traditional computers, embedded devices and in the browser. I have also added an index which helps to navigate the (perhaps long) document.

liscio commented 1 year ago

Thanks for addressing those bits. Everything looks good from where I stand. 👍

JorenSix commented 1 year ago

Thanks @liscio for the review, I am looking forward to the comments by @ebezzam. Thanks!

bmcfee commented 1 year ago

Checking in - @ebezzam, will you have a chance to look at this soon?

ebezzam commented 1 year ago

Review checklist for @ebezzam

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ebezzam commented 1 year ago

Checking in - @ebezzam, will you have a chance to look at this soon?

yes sorry just got back from holiday and should be able to finish by next week!

ebezzam commented 1 year ago

@JorenSix, @bmcfee

From my initial review, there are a couple blockers:

Minor:

JorenSix commented 1 year ago

@ebezzam thanks for taking the time for the review!

bmcfee commented 1 year ago

Thanks @ebezzam !

  • I don't know how this fits with JOSS ethics on self-plagiarism. Maybe @bmcfee can clarify?

I think this is fine and in accordance with the ethics guidelines. As @JorenSix notes, ISMIR Late-breaking demos (LBDs) are not peer-reviewed and are not published in the proceedings. It's quite common for preliminary work to first appear as an LBD and later be published properly (at ISMIR or elsewhere).

The minor comment is noted. I am not sure about the best way to reference software in latex but I will look into it.

If the software being cited does not have a DOI, the JOSS submission guidelines give an example of how to do this: https://joss.readthedocs.io/en/latest/submitting.html#example-paper-and-bibliography - excerpt:

@misc{fidgit,
  author = {A. M. Smith and K. Thaney and M. Hahnel},
  title = {Fidgit: An ungodly union of GitHub and Figshare},
  year = {2020},
  publisher = {GitHub},
  journal = {GitHub repository},
  url = {https://github.com/arfon/fidgit}
}
ebezzam commented 1 year ago

Thanks @JorenSix and @bmcfee for the quick responses and clarifications. I've opened a few issues on minor things / quick fixes.

On the unchecked points of my review:

Feature request, which I think could really enhance usability and usage, is a Python wrapper. Like this for SoX. I'll let @bmcfee weigh in if he thinks this is necessary for acceptance.

Hope that helps!

bmcfee commented 1 year ago

Thanks @ebezzam !

I don't think a python wrapper is necessary here, though it would of course be a nice feature at some point.

JorenSix 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:

JorenSix commented 1 year ago

Thanks for the feedback! I have been busy and Olaf has improved quite a bit! The changes:

I agree that a python wrapper and packaging (for apt, yum, pacman, ...) are two nice-to-haves which would increase adoption of Olaf but I see this as separate from the core Olaf software (as is the case for the pysox).

Thanks again for taking the time to review this paper/software and please let me know if there are additional points to tackle!

ebezzam commented 1 year ago

@JorenSix thanks for the updates to the repo and paper! I've opened a few "issues" on some minor points.

JorenSix commented 1 year ago

Thanks @ebezzam for the additional pointers. Now these small issues have been fixed and the paper and docs have improved!

JorenSix commented 1 year ago

@editorialbot generate pdf

JorenSix commented 1 year ago

Just an FYI, I have improved the speed of Olaf significantly with an algorithmic optimisation in the max-filter step. After a profiling session It became clear that this was the main speed bottleneck. Now it implements a (partial) Van Herk max filter as described in Van Herk, M. (1992). A fast algorithm for local minimum and maximum filters on rectangular and octagonal kernels. Pattern Recognition Letters, 13(7), 517-521.

The Panako vs Olaf results have changed and this is now reflected in the graphs/texts. @ebezzam , @bmcfee let me know if there are additional points or the we can move on ;).

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:

bmcfee commented 1 year ago

Thanks for the updates! I'm OOO for a few days, but will be able to check back in on this next week.

ebezzam commented 1 year ago

@JorenSix thanks for quickly addressing the points! Paper is looking much more complete, and great that you could resolve this bottleneck! Just one more formatting :see_no_evil: https://github.com/JorenSix/Olaf/issues/35

JorenSix 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:

JorenSix commented 1 year ago

The formatting of the author names is fixed, the use of authors={{auhor list}} made latex quote the long author list literally. The use of authors={author list} fixed it.

Thanks again @ebezzam for spotting the issue!

ebezzam commented 1 year ago

thanks @JorenSix, everything checks off for me!

JorenSix commented 1 year ago

I have just pushed another performance improvement (SIMD and a conditional which avoids a performance bottleneck) together with new Panako vs Olaf results. Olaf is about ten times faster than before the performance improvements, while all the rest remained the same. See here for more background on the performance optimizations.

@bmcfee It would be great to be able to round things off :)

bmcfee commented 1 year ago

@editorialbot generate pdf

bmcfee commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1109/MMSP.2002.1203273 is OK
- 10.5281/zenodo.1416340 is OK
- 10.5281/zenodo.1417593 is OK
- 10.1109/TASL.2012.2234114 is OK
- 10.1007/978-3-319-73165-0_10 is OK
- 10.1007/s11265-005-4151-3 is OK
- 10.5281/zenodo.1414728 is OK
- 10.1121/1.400476 is OK
- 10.1121/1.404385 is OK
- 10.1109/CDMA47397.2020.00010 is OK
- 10.1145/3144749.3144755 is OK
- 10.1109/ICASSP39728.2021.9414337 is OK
- 10.5281/zenodo.1417008 is OK
- 10.5281/zenodo.1416190 is OK
- 10.1007/s12193-015-0196-1 is OK
- 10.48550/arXiv.2302.12258 is OK
- 10.5281/zenodo.1422385 is OK
- 10.5281/zenodo.1422385 is OK
- 10.5281/zenodo.1417973 is OK
- 10.5281/zenodo.4245544 is OK
- 10.21105/joss.04554 is OK
- 10.1007/s12193-015-0196-1 is OK
- 10.5281/zenodo.7343030 is OK
- 10.1007/978-3-031-18444-4_16 is OK
- 10.1007/978-3-031-07015-0_16 is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

bmcfee commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

bmcfee commented 1 year ago

Thanks again @ebezzam and @liscio for your reviews!

@JorenSix - the bot has created a post-review checklist for us to complete before the paper is accepted. (The steps should be familiar from your previous submission, but now we have a way to track them!)

Let me know as you complete the "author tasks" steps on your side, and I'll get to work on the editor tasks.

JorenSix commented 1 year ago

See below:

Also from me a thanks to @ebezzam and @liscio for the constructive feedback. It certainly improved Olaf!

bmcfee commented 1 year ago

I read through the paper, and have a few comments:

General comment: in the bibliography entries, many parts that should be capitalized are not, e.g., "Lmdb". You can force proper capitalization by embedding the appropriate parts in curlies, eg, "{LMDB}: A general-purpose ..." .

Citation for Lòpez-García et al., seems to have an awkwardly formatted DOI (ie it's URL-encoded). Please check this one.

bmcfee commented 1 year ago

@editorialbot set v1.1.0 as version

editorialbot commented 1 year ago

Done! version is now v1.1.0

bmcfee commented 1 year ago

@editorialbot set 10.5281/zenodo.8093527 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8093527

JorenSix 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:

JorenSix 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:

JorenSix commented 1 year ago

@bmcfee Thanks for the pointers.

I fixed the capitalization issues in the bibliography (PFFFT, LMDB, inconsistent title capitalization), the DOI, introduced MIR and expanded on the microcontroller specs.

bmcfee commented 1 year ago

Thanks @JorenSix - i just noticed one more bib entry with a url-encoded DOI (Cortès et al). Everything else looks good to me.