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]: S(ound)lab: A teaching-oriented Python package for designing and running psychoacoustic experiments. #3284

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @DrMarc (Marc Schönwiesner) Repository: https://github.com/DrMarc/slab Version: v0.9.0 Editor: @arfon Reviewer: @hadware, @sneakers-the-rat Archive: 10.5281/zenodo.5029602

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

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

@hadware & @sneakers-the-rat, 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 @hadware

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @sneakers-the-rat

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. @hadware, @sneakers-the-rat 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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3758/s13428-018-01193-y is OK
- 10.25080/majora-7b98e3ed-003 is OK
- 10.1038/s41586-020-2649-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.36 s (99.4 files/s, 20803.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          17            497           1683           3480
reStructuredText                 8            256            228            776
Markdown                         4             83              0            331
YAML                             4             10              3             66
TeX                              1              6              0             62
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            36            864           1922           4750
-------------------------------------------------------------------------------

Statistical information for the repository '3a528629994e4938bced8c75' was
gathered on 2021/05/14.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Marc                             2             0             18            0.08
Marc Schoenwiesner             219          9413           5493           69.37
Ole Bialas                      16          4205           2358           30.54

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
Marc Schoenwiesner         2589           27.5         14.7                3.48
Ole Bialas                 3071           73.0          1.9               11.01
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:

arfon commented 3 years ago

@hadware, @sneakers-the-rat - 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/3284 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.

sneakers-the-rat commented 3 years ago

hope it's alright if i do my checklist in a separate comment & copy/paste it into the OP at the end so i don't accidentally mess up anything @hadware is doing.

I'll use these prefixes so you can ctrl-f them

If i leave comments on a checkbox section but still check the box that means i'm fine with that section but just have some suggestions/clarifying questions.

note that i'm going to do this in a couple sittings so don't be alarmed if things are unchecked for a bit. feel free to respond to any of this at any time but i make no claims that it isn't a total mess until i mark it finished ;)

Review checklist for @sneakers-the-rat

Conflict of interest

Code of Conduct

General checks

a naive whole-repo hercules ownership plot shows that the library is currently roughly a ~50/50 split between the two authors slab_burndown

Functionality

so far so good. can't believe that slab was still available on PyPI. like the minimal dependencies, good to see how much easier it's gotten to play sounds at an OS level from python.

*->> (fixed in https://github.com/DrMarc/slab/issues/37) soundcard seems like a major functionality point of the library. from the docs i would expect that it is installed by default, but it's not and there isn't a notice given to the user that it hasn't been found. I assumed it was being used until i tried to import it manually. I would recommend adding it as a default or else as a clear optional dependency, and warn the user if it can't be found if it's the expected default behavior. -

*?>> (fixed https://github.com/DrMarc/slab/issues/38) Curious about the h5netcdf and windows-ncurses dependencies being described in the docs rather than included as a default or optional dependency, for example like 

install_requires=[..., "windows-curses;platform_system=='Windows'"]
extras_require={
    'hrtf': 'h5netcdf'
}

*!>> I don't see a list of supported operating systems here, and see mentions of windows, linux & mac, it might be good to have specific installation sections for all supported operating systems, eg. how to install libsndfile with homebrew, etc.

first of all really impressed with the level of functionality here, and y'all should be proud of making a package that you can pip install and successfully play sounds (especially on a mac) without any additional system configuration -- in my experience that can be challenging!

though initially the class design of Sound was a little odd to me as it combines generation methods with processing methods, but i started to see the benefits of it and really like it. being able to chain method calls like Sound.tone().filter() will be super useful for being able to specify a complex sound programmatically, eg. something like Sound.from_list([{'type':'tone', 'frequency':500}, {'type':'filter', ...}]).

I can see it getting difficult to maintain as the class is already ~1250 lines long, and think a little bit of refactoring would make it easier on y'all, but that's a "good problem" to have for a library of this maturity b/c after all it's big because it does a lot of stuff.

just getting to the Binaural section and i'm very excited, really cool stuff -jls 21-05-14

finished up -- extremely impressed with this package. nice work!!!!! very impressed with the experimental module, the filter module, and the hrtf modules as much as i am with the base sound module.

Explicit claims in manuscript:

*->> (addressed in https://github.com/DrMarc/slab/issues/23) sounds of different durations can't be added + together. raised issue here: https://github.com/DrMarc/slab/issues/23

*!>> (fixed https://github.com/DrMarc/slab/issues/35) calibrations are automatically saved and loaded, which is nice (might be good to give notice that a calibration has been loaded on import) but default samplerates are not. It seems appropriate for this package, since it's intended as a modular tool for users to hack around with/reconfigure/etc., to consolidate user preferences into a single getter/setter method, maybe in a single .pck or .json or .yaml or whatever file so this is easier to maintain.

*!>> (fixed https://github.com/DrMarc/slab/issues/34 ) Binaural._apply_ild (probably just .ild) needs one more thing documented -- it takes the average of the existing levels of the sounds, and then uses that as the midpoint for the new levels, but it's ambiguous whether you would do that or anchor off of one of the existing levels. one sentence clarifying the behavior in the docs would help.

*!>> (addressed https://github.com/DrMarc/slab/issues/33 )waveform autoscales the y-axis which makes it a little difficult to compare across amplitudes. maybe make that optional?

*!>> (fixed https://github.com/DrMarc/slab/issues/32 )wrappers around functions with a lot of parameters should probably take kwargs and give them to the wrapped function. eg. spectrogram passes kwargs to axis.imshow but most of the other params are hard-calculated. would be good to be able to pass in spectrogram_kwargs to override default calculated values if wanted. totally optional QoL thing that would be pretty cheap to implement.

*!>> (fixed https://github.com/DrMarc/slab/issues/31) looks like the list of features in the .rst description of spectral_feature in the docs is broken

*->> the documentation says to call slab.calibrate() to calibrate sounds, but it is not imported to the top level of the module from slab/sound.py

*->> (fixed) slab.sound.calibrate() exits with an error, raised issue https://github.com/DrMarc/slab/issues/22

Other claims in manuscript:

Other assorted functionality comments

*!>>(addressed https://github.com/DrMarc/slab/issues/30 ) the separation of level from other parameters is odd, i would expect to be able to do slab.Sound.pinknoise(duration=0.5, level=60) in addition to setting it as a property after instantiation. seems like it would be a good idea to add a **kwargs to the @staticmethods that gets passed on to the Sound init method, if eg. a level is supplied it can be set there. would make maintainability of class-level features a bit easier.

*!>> (implemented) the implementation of the Sound.play method is a little opaque, i would expect a warning and a description of the fallback if soundfile wasn't available. eg. since the fallback is to write to a .wav file and play with other cli tools, that would be something the user should probably know given the possibility of inconsistent state problems from eg. two sounds trying to write/play from the same file. I would strongly recommend a hash-based naming system rather than a hardcoded name as in https://github.com/DrMarc/slab/blob/fb1f270df712f7b3ce89f3dede69dd1487061fa8/slab/sound.py#L879 , so you can ensure that different sounds don't conflict but also can take advantage of caching advantages (eg. if a file with a particular hash already exists, you don't need to spend the i/o to write it again). you can do something like hashlib.sha256(Sound.data).hexdigest() to generate the filenames (probably need to actually set a seed/etc.

*!>> (addressed https://github.com/DrMarc/slab/issues/29) strongly recommend to create a user directory that is outside the repository or library location, maybe in ~/slab as it makes it more difficult for new programmers to contribute/use the library and makes operation a bit more opaque. (where is my calibration file again?). also becomes a bigger problem when you aren't guaranteed permissions, eg in the case where someone installs it as a system package with sudo pip install slab

!>> recommend making an explicit test/wrapper functionality to avoid Binaural getting out of sync with Sound - https://github.com/DrMarc/slab/issues/41

!>> as someone going through this same thing right now, if you intend on developing experiments in the main repo then you'll want to eventually split them off or else freeze them as examples -- can get very messy to combine any active lab code with deployed code :)

Documentation

The need is clearly described in the manuscript -- a simple script-based psychoacoustics library that incorporates HRTFs. Big fan of modular tools and think that's the route that most scientific software should take, often attempts to, and frequently misses (myself included) so I am interested to see the implementation.

Other assorted docs suggestions:

*!>> (implemented) the individual pages in the docs are relatively long, and you've done a good job of writing headings in your .rst, so it would be a big usability improvement for very cheap to configure the docs to enable hierarchical navigation, see the docs here: https://sphinx-rtd-theme.readthedocs.io/en/latest/configuring.html#theme-options specifically setting collapse_navigation=False in conf.py (and adding sphinx-rtd-theme to an additional optional 'docs' install configuration and etc)

*!>> (not all that important) I think some of the introduction being split off to a FAQs page might be a good idea

!>> format of trialsequence could use some additional clarity, assorted observations of psychoacoustics docs (and then just eventually the whole psychoacoustics section) : https://github.com/DrMarc/slab/issues/42

!>> combined suggestions for filters docs and module here: https://github.com/DrMarc/slab/issues/43

->> data_path() is not found for hrtf: https://github.com/DrMarc/slab/issues/44

->> label is not found for Filter.tf(): https://github.com/DrMarc/slab/issues/45

Software paper

danielskatz commented 3 years ago

Please just use the checklist at the top.

DrMarc commented 3 years ago

"!>> the individual pages in the docs are relatively long, and you've done a good job of writing headings in your .rst, so it would be a big usability improvement for very cheap to configure the docs to enable hierarchical navigation..."

Good point! We enabled hierarchical navigation as recommended.

DrMarc commented 3 years ago

"I would strongly recommend a hash-based naming system rather than a hardcoded name"

The hashed file names are an excellent idea. It's implemented now.

DrMarc commented 3 years ago

Hi @sneakers-the-rat, I put your comments in issues and we'll respond there. That may make it easier to keep track of progress.

hadware commented 3 years ago

Alright, it's a pass for me! Really great job on this lib @DrMarc (and your acolyte @OleBialas). I'll be telling my labmates working on psychoacoustics about it :)

arfon commented 3 years ago

Alright, it's a pass for me! Really great job on this lib @DrMarc (and your acolyte @OleBialas). I'm be telling my labmates working on psychoacoustics about it :)

:zap: thanks @hadware!

DrMarc commented 3 years ago

Many thanks @hadware for taking the time to evaluate the module!

DrMarc commented 3 years ago

@arfon - Ole and I would like to indicate that each of us contributed 50% to the work (sometimes called shared first authorship). Is there a preferred way to do that at JOSS?

arfon commented 3 years ago

See the example docs on how to do this (https://joss.readthedocs.io/en/latest/submitting.html#example-paper-and-bibliography). Basically add ^[co-first author] after each of the names.

DrMarc commented 3 years ago

Excellent, thanks! I did not see that.

whedon commented 3 years ago

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

DrMarc commented 3 years ago

Hi @sneakers-the-rat! From our side, we have addressed all comments so far (see resolved issues in the repo), except for the spectrogram kwargs suggestion, which I will implement later today. Your point about the data directory uncovered a major flaw and lead to the longest internal debate, and I think we have found a good solution there. All the smaller points were also really helpful and have helped to improve the code a lot.

sneakers-the-rat commented 3 years ago

Sorry for the delay! going to continue my review now and will try to have it finished early next week latest. I don't anticipate having any major complications, just want to make sure i actually make it through the whole package! Excited to see the discussion/what you've come up with re ^^

sneakers-the-rat commented 3 years ago

whup whup @DrMarc and @OleBialas this is a big pass from me!

I left some last (optional) comments in the final issues that might be helpful:

really excellent work! love this package, after i got a hang on the design i really enjoyed using it while testing it out. will absolutely recommend this to anyone looking for a simple to use but powerful psychoacoustics package!

DrMarc commented 3 years ago

Hi @sneakers-the-rat, Many thanks for the details comments and bug-hunting! This is very valuable. We will correct and expand the docs, as you suggested, and take care of the other issues. Some of these issues are intuitive to us, but may not be to others. The multi-channel filter is an example (#43). We work with multi-channel sound (48-channel speaker array in the lab), and our typical use case is applying each filter channel to each sound channel. But I can see that the sequential application may be useful in other scenarios. Thanks again for your work on this!

DrMarc commented 3 years ago

Hi @arfon , we have now addressed the optional suggestions by @sneakers-the-rat. I think this completes the revision.

arfon commented 3 years ago

@DrMarc – At this point 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.

DrMarc commented 3 years ago

This is the Zenodo DOI with the same name and authors: 10.5281/zenodo.5029602

DrMarc commented 3 years ago

@arfon - sorry, forgot to tag you. See above.

arfon commented 3 years ago

@whedon set 10.5281/zenodo.5029602 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5029602 is the archive.

arfon commented 3 years ago

@whedon recommend-accept

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.3758/s13428-018-01193-y is OK
- 10.25080/majora-7b98e3ed-003 is OK
- 10.1038/s41586-020-2649-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2417

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2417, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 3 years ago

@DrMarc – could you please merge this PR before we proceed? https://github.com/DrMarc/slab/pull/47

DrMarc commented 3 years ago

@arfon - Done!

arfon commented 3 years ago

@whedon recommend-accept

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.3758/s13428-018-01193-y is OK
- 10.25080/majora-7b98e3ed-003 is OK
- 10.1038/s41586-020-2649-2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2418

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2418, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
arfon commented 3 years ago

@whedon accept deposit=true

whedon commented 3 years ago 
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 3 years ago

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

whedon commented 3 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/2419
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03284
  3. If everything looks good, then close this review issue.

  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

    Any issues? Notify your editorial technical team...

DrMarc commented 3 years ago

@arfon - very nice! Thanks for the quick action and super friendly review. This was by far my most pleasant review experience. I'd be up for reviewing, in case you need people. Thanks again -Marc

arfon commented 3 years ago

@hadware, @sneakers-the-rat – many thanks for your reviews here! JOSS relies upon the volunteer effort of people like you and we simply wouldn't be able to do this without you ✨

@DrMarc – your paper is now accepted and published in JOSS :zap::rocket::boom:

@arfon - very nice! Thanks for the quick action and super friendly review. This was by far my most pleasant review experience. I'd be up for reviewing, in case you need people.

Thank you! Whedon will provide some information below in just a moment explaining how you can sign up as a potential reviewer 👇 ✨

whedon commented 3 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.03284/status.svg)](https://doi.org/10.21105/joss.03284)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.03284">
  <img src="https://joss.theoj.org/papers/10.21105/joss.03284/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.03284/status.svg
   :target: https://doi.org/10.21105/joss.03284

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following: