search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

baRulho #609

Closed maRce10 closed 3 months ago

maRce10 commented 9 months ago

Date accepted: 2024-03-28 Submitting Author Name: Marcelo Araya-Salas Submitting Author Github Handle: !--author1-->@maRce10<!--end-author1-- Repository: https://github.com/maRce10/baRulho/ Version submitted: 2.1.0 Submission type: Standard Editor: !--editor-->@jhollist<!--end-editor-- Reviewers: @mikemahoney218, @DenaJGibbon

Archive: TBD Version accepted: TBD Language: en

Package: baRulho
Type: Package
Title: Quantifying (Animal) Sound Degradation  
Version: 2.1.0
Author: Marcelo Araya-Salas [aut, cre]
Maintainer: Marcelo Araya-Salas <marcelo.araya@ucr.ac.cr>
Description: Intended to facilitate acoustic analysis of (animal) sound transmission experiments, which typically aim to quantify changes in signal structure when transmitted in a given habitat by broadcasting and re-recording animal sounds at increasing distances. The package offers a workflow with functions to prepare the data set for analysis as well as to calculate and visualize several degradation metrics, including blur ratio, signal-to-noise ratio, excess attenuation and envelope correlation among others (Dabelsteen et al 1993 <doi:10.1121/1.406682>).
License: GPL (>= 2)
Imports: utils, stats, seewave, tuneR, fftw, methods, viridis, Sim.DiffProc, png, checkmate, cli
Depends: R (>= 3.2.1), warbleR (>= 1.1.29), ohun (>= 1.0.0)
LazyData: TRUE
URL: https://github.com/maRce10/baRulho
BugReports: https://github.com/maRce10/baRulho/issues
Suggests:
    rmarkdown,
    ggplot2,
    knitr,
    kableExtra,
    testthat (>= 3.0.0),
    covr,
    formatR,
    Rraven
VignetteBuilder: knitr
RoxygenNote: 7.2.3
Repository: CRANs
Language: en-US
Authors@R: person("Marcelo", "Araya-Salas", role = c("aut", "cre"), email = "marcelo.araya@ucr.ac.cr")
Config/testthat/edition: 3

Scope

It estimates degradation measures from sounds that have been re-recorded in standard playback experiments. It also helps to prepare acoustic data for playback experiments.

Scientific community working with bioacoustic data. Useful to automate analysis of animal acoustic signals for a variety of research questions (e.g. behavior, ecology, evolution, conservation)

Somehow continuous integration is not detected when running pkgcheck::pkgcheck(), but it is fully implemented https://app.codecov.io/gh/maRce10/baRulho?branch=master

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

MEE Options - [x] The package is novel and will be of interest to the broad readership of the journal. - [x] The manuscript describing the package is no longer than 3000 words. - [x] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

jhollist commented 5 months ago

Thanks, @DenaJGibbon and @mikemahoney218

maRce10 commented 5 months ago

Many thanks for the review. I am really grateful for all your time and effort. I have made changes and responded to your comments/suggestions below, quoting the commit the change was made in.

Response to @mikemahoney218

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/609#issuecomment-1785830265

comment: My main general comment about the package is, as Jeff and the automatic checks both identified, many functions in this package have a pretty high cyclomatic complexity. As an outside reviewer, this made it hard to trace any given function. I'd encourage the developers to break their functions down into more modular pieces, in order to possibly be able to reuse elements across functions but also to make the package easier to scan and maintain in the long run.

response: I have broken down functions and now all functions have a cyclomatic compelxity lower than 15 maRce10/baRulho@b6868c4e41646927e5e81f3b4e7e291e24bc6ab7

comment: I saw the earlier conversation between you and Jeff on this, but I think the size isn't due to using sound examples -- I think it's due to the size of images you're using in your help files and vignettes. It seems like the PNGs in the package are saved at 300 PPI resolution; saving them at 72 or 96 PPI instead will save a ton of space (for instance, it shrinks the package sticker from 1.4 MB to 162.5 kB on my machine) while leaving them high-res enough for computer displays. You might also consider moving the quantify_degradation vignette to be an "article" instead, saving some of the space that gets used for those images.

response: I have reduce the size of the sticker to 162.5 kB as suggested by the reviewer. (maRce10/baRulho@9c9d95dd425d50f1e0258b412f887779619657e5). I also remove 4 images from the vignettes as part of having all chunks evaluated (maRce10/baRulho@176266b43a73bcd01b16b59dc5c42abf26ee75ff & maRce10/ohun@49d29ba12f5f48d71683b5266633f54e8dc530f0). _

Vignettes

align_test_sounds.Rmd

comment: In general, I'd encourage you to evaluate all chunks in a vignette unless they're expected to error, on purpose, and this is highlighted to the reader. I get into specifics below, but reading the rendered vignette I am confused by what some functions are meant to output, and I believe there are some code errors hidden in unevaluated chunks.

response: Now most vignette chuncks are evaluated, except for those that create image file which are shown in a gif or those with routines that take a considerable amount of time to run maRce10/baRulho@176266b43a73bcd01b16b59dc5c42abf26ee75ff & maRce10/baRulho@7fb6a5f7af1c7e9c456cc79e6268c76a787b632b

comment: This vignette defines td in a setup chunk that's not included in the vignette, then uses it in user-visible code throughout the vignette. As a user, I'd expect to be able to run the code displayed in the vignette. I'd suggest either defining td in the user-visible part of the vignette, or using tempdir() as an argument to these functions, rather than using the variable.

_response: I have followed the recommendation by the reviewer to use tempdir() maRce10/baRulho@176266b43a73bcd01b16b59dc5c42abf26ee75ff

comment: The calls to spectrograms() and check_sels() using synth_master_annotations error ("NAs found in bottom.freq and/or top.freq"). Those chunks are marked as eval = FALSE, so maybe this is known, but there's nothing in the vignette implying that these chunks will error -- in fact, there are spectrograms included below the chunk, which mean I expect that running the code in that chunk will produce the resulting graph.

response: I have fixed these bugs. Now those code chunks are evaluated. I also decided to use a different spectrogram creating function (seewave::spectro()) that plots the spectrograms in the R graphic device, which is more intuitive maRce10/baRulho@176266b43a73bcd01b16b59dc5c42abf26ee75ff

comment: Is there any reason you switch from using arguments to set file paths to using options right before find_markers()? The inconsistency confused me.

response: options() is not longer used maRce10/baRulho@7fb6a5f7af1c7e9c456cc79e6268c76a787b632b

comment: You have a hidden call to print(TRUE) after calling is_extended_selection_table(). I think it would probably be better to return the actual output from is_extended_selection_table() instead. On my machine, this returned FALSE, making the vignette incorrect (the actual class is data.frame).

_response: _It has been removed and is_extended_selectiontable() is evalauted maRce10/baRulho@7fb6a5f7af1c7e9c456cc79e6268c76a787b632b

comment: The command \code{\link{align_test_files}} in the manually fixing alignment section produces a blank space in the HTML output. I think this should probably be align_test_files().

response: fixed maRce10/baRulho@58402dd843a35da85bf9779aa29652be378b1f09

comment: In auto_realign() I'm seeing:

Warning messages: 1: In attr(X, "check.results")$start[paste(X$sound.files, X$selec, : number of items to replace is not a multiple of replacement length 2: In attr(X, "check.results")$end[paste(X$sound.files, X$selec, sep = "-") %in% : number of items to replace is not a multiple of replacement length

Is that expected?

response: Nope. I fixed it maRce10/baRulho@b47aef37d3f1b09f17f4003b1790c1ac765a42e7

quantify_degradation.Rmd

comment: options(dest.path = td) (in the blur ratio section) errors because td isn't defined. As with the other vignette, the mix of hidden evaluated code and unevaluated displayed code means the actual code displayed in the vignette doesn't always work.

response: I have removed the use of options() and set all arguments on the function call maRce10/baRulho@598b18ed40db6f7ee3003da0ffa77e4470e3c9af

comment: There's a line in this vignette that says "The following code sets env.smooth = 800", but it's not obvious to me where or why that gets set.

_response: We have added env.smooth = 800 to the blurratio call and added that 'which produces smoother envelopes' maRce10/baRulho@3a8c41a691f6d41e23ccf788b8f9fe6bb48e3774

comment: Some of the plots use scale_color_viridis(), some use scale_color_manual(values = viridis::viridis()), and it's not obvious to me why this changes. It makes comparing the code in different chunks hard, as there are a number of things changing for reasons I can't identify.

response: Fixed. Now all ggplot color scales use viridis directly maRce10/baRulho@7e22e55b795f3f58de150035735a7ab05a50bba2

comment: I don't know if it's your CSS formatting or something else, but while code in the Rmd is formatted nicely, the ggplot2 code on the pkgdown site seems to all be on a single line in a way that makes it hard to parse.

_response: I used optschunk$set(tidy = TRUE) but doesn't seem to work well. So I formatted the code myself (F2) maRce10/baRulho@7e22e55b795f3f58de150035735a7ab05a50bba2

Examples

comment: If you're going to use \dontrun around examples that should work, which don't highlight to users that they're expected to fail, I recommend using devtools::run_examples(run_donttest = TRUE, run_dontrun = TRUE) to confirm that the code actually will run successfully.

_response: I used devtools::run_examples(run_donttest = TRUE, run_dontrun = TRUE) to spot problematic examples and fix them accordingly maRce10/baRulho@d372f71da19f20c8dfd3f8bfa963c1aca5940e44. Note that manualrealign is an interacitve function

comment: blur_ratio.R: need to use ggplot2:: or library(ggplot2). It's probably good practice to also wrap this in if (requireNamespace("ggplot2", quietly = TRUE)) (or, if you're willing to pick up a dependency on rlang, rlang::check_installed("ggplot2")).

_response: I added rlang::checkinstalled("ggplot2") and removed \dontrun maRce10/baRulho@2be41e78e675d428e543f43b8340a318b4ddf246

comment: manual_realign.R: Error: object 'master_df' not found. Seems like Y should be as.data.frame(master_est) (or this object needs to be created earlier)? manual_realign.R: Error in eval(ei, envir) : object 'mako' not found. Need to namespace viridis::mako().

_response: masterdf error fixed in maRce10/baRulho@d372f71da19f20c8dfd3f8bfa963c1aca5940e44. mako changed to viridis::mako() maRce10/baRulho@6459466444ae5431a13d1f7a69f045fce9b1503e

comment: spectrum_blur_ratio.R: also needs ggplot2:: or library(ggplot2).

_response: I added rlang::checkinstalled("ggplot2") maRce10/baRulho@2be41e78e675d428e543f43b8340a318b4ddf246

comment: Warning messages: 1: In attenuation(f = 2000, dist = 50, dist0 = 1) : partial argument match of 'f' to 'frequency' 2: In match.call(definition, call, expand.dots, envir) : partial argument match of 'f' to 'frequency' 3: In attr(X, "check.results")$start[paste(X$sound.files, X$selec, : number of items to replace is not a multiple of replacement length 4: In attr(X, "check.results")$end[paste(X$sound.files, X$selec, sep = "-") %in% : number of items to replace is not a multiple of replacement length 5: In eg$dur : partial match of 'dur' to 'duration' 6: In eg$harm : partial match of 'harm' to 'harmonics' 7: In eg$harm : partial match of 'harm' to 'harmonics'

response: maRce10/baRulho@e1fdd34736a525d48ef6222bab13f0ee1b9fd8cc

comment: blur_ratio.R: Any reason the call to blur_ratio() on line 42 is commented out?

response: Not really. I uncommented it maRce10/baRulho@e1916dad1da71d8d405704d84290ce2e5b87b50f

comment: Is there a reason you're using the @usage tag? Normally this section is auto-generated by rOxygen2, ensuring it stays up-to-date and properly formatted.

response: Did not know rOxygen2 can do it. I removed all function @usage fields maRce10/baRulho@9582da689ccd92e9861c81827de1d5f3b658f625

comment: I love the standardization of arguments across functions; I think that's a clear strength of the package. You might consider using rOxygen2 templates (see the relevant section of the packaging guide) to centralize these arguments' documentation, in order to reduce the amount of places you need to touch to update any documentation.

_response: I always wanted to do this but was not sure how. I added argument templates to a new R code file template_params.R which is now referenced to by other functions using @inheritParams templateparams maRce10/baRulho@0852fc0594de8cc0ceda438ae75021aafb2e3d91

comment: It might be nice to define more of the constants used in attenuation(), so that the calculations are a bit easier to interpret. Right now, there's a lot of magic numbers in this function call.

response: The complexity of the calculations makes difficult to explain those numbers so I pointed out to the original sources and added comments to calculations done in the function. maRce10/baRulho@3610d04e995051a0baeb324420a91d7b2d52ceb2

comment: In attenuation(), I think dist0 and dist are required inputs, but are placed after optional arguments. It might be nicer to place these arguments after frequency, to allow users to pass the distance arguments via position.

response: I placed dist0 and dist after frequency maRce10/baRulho@2cad8a200bafa1a816ea78f780d57ff638276034

comment: I think this is probably the easiest time to get rid of deprecated arguments and functions, before the package gets to CRAN/gets more visibility via the rOpenSci organization. I see the parallel argument and output argument in several functions is deprecated, and I think this is a good time to drop those, as well as the search_templates() and spcc_distortion() functions.

response: I get rid of all deprecated arguments and functions maRce10/baRulho@26a66796a75471e69d4d05c4383affffab2f42ed

comment: auto_realign(): I'd recommend providing the list of options to wn here. See the Enumerate possible options in the "Tidy design principles" book for an example of why this is useful.

response: Done maRce10/baRulho@8b4ce4e733ad67aad8a83310547395bf4b931c0f

comment: I'd similarly suggest using enumerations for noise.ref, type, method, and other functions where users must provide an option from a pre-defined list. Then use arg.match() or rlang::match_arg() to ensure provided values match one of the options.

response: Done maRce10/baRulho@d382a7f3e1d1b359442137b9c56102c90d147b55

comment: blur_ratio(), plot_aligned_sounds(): I think it'd make sense to mention in the help file that wl must be even, and that it will be adjusted if not. Also, does wl need to be an integer value? It's documented as a numeric vector, but any decimal values will have 1 added to them.

response: Now it states that "@param wl a vector with a single even integer number specifying the window length of the spectrogram, default is \code{NULL}. If supplied, 'hop.size' is ignored. Odd integers will be rounded up to the nearest even number." maRce10/baRulho@c08e626b6438a5ec931e40276af54f13b6e8aec5

comment: set_reference_sounds(): I see that this function requires eight columns with specific names. Is there any reason to not let the user control, via function arguments, which columns are used?

response: The columns are required for the function to set which sounds will be used as reference. Most of those columns are also used by other functions, so it is very convenient to standardize the names of the columns.

comment: Consider having functions that generate plots (I'm thinking of plot_degradation(), plot_blur_ratio()) return the file paths they generated. I was surprised a few times when a function appeared to not return anything, because it had written a figure out to disk instead.

response: I added a message to indicate where the files where saved and the functions also return the file paths invisibly (without printing). maRce10/baRulho@3bf53d80869f1f3111dbf859b9cf6629c0ce5312

comment: plot_blur_ratios(): colors says it should be a Character vector of length 4, but then the default argument is viridis::viridis(3), of length 3. Not sure what's correct here.

response: Fixed: "@param colors Character vector of length 4 containing the colors to be used for the color to identify the reference sound (element 1), the color to identify the test sound (element 2) and the color of blurred region (element 3)" maRce10/baRulho@c18f56a1382088d4cfeaa4dd25752168910b7075

comment: tail_to_signal_ratio(): bp having a default value that doesn't have the same type or length as what users would provide is a bit confusing. Maybe this default should be NULL?.

response: I decided to keep it as it is because the function let users to change the default value ("frange") which uses the actual frequency range in the input data to specific ranges

comment: type seems to be used in different ways in several different functions, with different sets of options in each. Consider using different argument names for each.

_response: I renames type to snr.formula and tsr.formula signal_to_noise_ratio and tail_to_signalratio respectively maRce10/baRulho@9c3980709ffe56e8c8ce364bba516f76e728812b

comment: The contributing guide seems to be missing an email:

You must never report security related issues, vulnerabilities or bugs including sensitive information to the issue tracker, or elsewhere in public. Instead sensitive bugs must be sent by email to <>. I'd either provide an email or drop this part of the guide.

response: Done maRce10/baRulho@ffe7432898165c513bbc85c2cbe04b3cb0bccd58

Response to @DenaJGibbon

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/609#issuecomment-1879632688

comment: For this round of comments I focused mostly on the vignette. I was able to work through all of the examples without any issue. However, there is one component that is missing from this. I think it will be important to add a section or separate page that focuses on how to run this using your own dataset. For example, if folks start with Raven selection tables, sounds files, and metadata of distance and other information, how do they get started using these methods? The examples in the vignette work great, but I think some work could be done to help people get their own data in the pipeline with this.

_response: The "aligned_test_sounds.Rmd" vignette uses actual recordings as those that people will get in their own experiments:

"We will use acoustic data included in the package as example data. This is a data subset from a sound transmission experiment. The complete data set of the experiment is hosted in this repository"

"Their names are self-explanatory: a master sound file (“master.wav”) and its annotations, a reference sound file (“reference_1m.wav”) and two test files (“test_10m.wav” and “test_20m.wav”) re-recorded at 10 and 20 m respectively.

Now we will save those files in a temporary working directory (those objects are extended selection tables in the R environment) to resemble the usual case in which the acoustic data is found in sound files."

I have added that: "Note the 'sound.id' column which contains a unique identifier for each sound. This will be used to label their counterparts in the test sound files. Users can import annotations from Raven using the Rraven package (see this vignette)." maRce10/baRulho@f17d56aa07235325083afe2221be928ba4d47460_

comment: It would be helpful to include an example or type of research question. This may be one of the first exposure people have to this type of work, so a few examples would be helpful

response: I added that "These experiments aim to answer research questions such as: How habitat structure has shaped the transmission properties of animal acoustic signals? Which acoustic features are shaped by selection for improving transmission? What is the active space of a acoustic signal?" maRce10/baRulho@6d8cc6d51124115e05fb94d835cfa6b5ff8b8c15

comment: What does the check part mean? A figure legend describing the diagram would be helpful.

response: I slightly modified the diagram, adding the related functions to it. So I ended up removing the "check" part. I also added the following legend: "Figure 1. Diagram depicting a typical workflow of experiments working on signal tranmission and degradation. Nodes with black font indicate steps that can be conducted using baRulho functions. Blue nodes denote the functions that can be used at those steps." maRce10/baRulho@95ba615f22873c12ad914cbb475b687b1fbb9086

comment: I was quite impressed with how smoothly I was able to work through this section of the vignette- nice work!

response: thanks!

comment: You might want to include a warning about playing the sound synthetic master file (!). It gave me quite a shock!

response: I added: "(be careful when playing the master sound file as it can be shocking for some audiences!)" maRce10/baRulho@78c0526d1699d7a36c53db15eb2fb2334530d522

comment: I can imagine there will be times when spectrogram cross-correlation won't work to time align files. Perhaps adding something about using caution and the need to manually check would be good.

response: I have clarified that "In some cases alignments might not be accurate, particularly when markers have been considerably degraded on the test sound files" maRce10/baRulho@

comment: I realize this is an R package vignette and not a peer-reviewed paper, but I think including some pros/cons of different measures would be helpful. How should someone who is coming to this for the first time decide which values to measure and report?

response: I am working on the manuscript at this point, which touches upon this issues. I will refer to the paper once it is publish.

comment: For all of the different types of degradation measures it would be good to include what units they are in (e.g. decibels, etc), as this will facilitate folks who want to use this for publications and to report their results using best practices.

response: I added units to the @return filed for those non-unitless measures maRce10/baRulho@ce2e2953115fd12d81aaf4afbedd445f831847ae

comment: “However, if there is another test sound from the same ‘sound.id’ at a shorter distance in other transects, it will be used as reference instead.” Should this be the default? What if transects are very different?

response: In this type of experiments reference sounds are typically recorded at 1 meter without any vegetation in between to account for speaker and recording equipment effects, although habitat has little effect given the short distance. Furthermore, results from transects that use references from different distances would not be comparable. I clarified that: "However, if there is another test sound from the same 'sound.id' at a shorter distance in other transects, it will be used as reference instead. This behavior aims to account for the fact that in this type of experiments reference sounds are typically recorded at 1 m and at single transect." maRce10/baRulho@09d3d4edb6e5f6bf76b5e3016598952ac31d2c1b

comment: “The function also checks that the information ‘X’ is in the right format so it wont produce errors in downstream analysis (see ‘X’ argument description for details on format).” I am not sure what the X argument is?

response: I clarified that "... the information 'X' (the input annotation data)..." maRce10/baRulho@caff65e31ea7767b6539c4aee1fb0642bf7b3499

comment: Typo? Right above ‘blur ratio’: “Before This is one the plots generated using the example data”.

response: Fixed. maRce10/baRulho@24f3dd525ee1eb5650a3e57823c3e645e944d76d

comment: “The env.smooth argument could change envelope shapes and related measurements. The following code sets env.smooth = 800:” Could you provide more information on this, why would you want to change it, and what would the expected changes be?

response: I clarified that: "The env.smooth argument could change envelope shapes and related measurements, as higher values tend to smooth the envelopes" maRce10/baRulho@95e4472c360ed8f34de40ad305a0647134d6dfe6

comment: Can you include a citation for ‘spectrum blur ratio’?

_response: I created that measure and has not been published yet.

comment: For both blur ratios, it would be good to briefly explain how you calculate the mismatch.

_response: I think it is already explained "Blur ratio is measured as the mismatch between amplitude envelopes (expressed as probability density functions) of the reference sound and the re-recorded sound. Low values indicate low degradation of sounds.". I also include graphs showing the blur ratio, which are, to my knowledge, the first visualizations of such a measure.

comment: SNR can be measured many different ways (e.g. from waveform or spectrogram). I think there need to be a few lines added here explaining which approach is used to measure SNR.

_response: I clarified that: "This method is implemented in the function signal_to_noise_ratio(), which uses envelopes to quantify the sound power for signals and background noise" maRce10/baRulho@547c138f9c37e1c8065eb1c7d0d3e52ce3cc25d8_

comment: Some of the SNR values in the example seem strange, such as -37? More detail on how it is calculated and what is actually reported will be helpful here.

response: THe formulas are explained in the function documentation. I added to the vignette that: "Negative values can occur when the background noise measured has higher power than the signal" maRce10/baRulho@20a5fde6b49ceb1e9cdce4622adfdac2bebe6919

comment: What should the range of TSR be? All the examples are negative.

response: Not sure about all possible values as this is a ratio. I added that: "Tail-to-signal ratio values are typically negative as signals tend to have higher power than that in the reverberating tail." I also added this to the function documentation maRce10/baRulho@382887671443a273ae987ef4486e132f93036f78

comment: The references info is missing from this page

response: You mean the reference section? I see it there.

maRce10 commented 5 months ago

@jhollist just wondering if the response to the reviews was noticed :smile:

jhollist commented 5 months ago

@maRce10 Sorry about that! Got lost in my GitHub notifications!

@DenaJGibbon and @mikemahoney218 Could you please take a look at @maRce10 revisions. If you are satisfied with the revisions let me know and if there are still some things to work thought let us know as well. Getting close on this one! Thanks all.

ropensci-review-bot commented 5 months ago

@maRce10: please post your response with @ropensci-review-bot submit response <url to issue comment> if you haven't done so already (this is an automatic reminder).

Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html

mikemahoney218 commented 5 months ago

Just want to mention I deleted an earlier comment, in case anyone got an email about it -- I think my local version of this project got into a really messy state. Continuing my re-review now!

mikemahoney218 commented 5 months ago

Is your pkgdown website not rendering anymore? The version on the website is out-of-date from your actual Rmd files, and contains a few errors that aren't present in the Rmd.

In the alignment vignette, warbleR::is_extended_selection_table(aligned_tests) still returns FALSE for me (when running only user-visible code), while a hidden chunk prints TRUE. Not sure what's going on here -- the fact that this apparently should be an extended selection table, but isn't, feels concerning? But I have no idea if this is actually a problem.

I'm still seeing one error in examples:

# add_noise.R
> data("test_sounds_est")

> # make it a 'by element' extended selection table
> X <- warbleR::by_element_est(X = test_sounds_est)
  |                                                  | 0 % ~calculating  Error in wave[a:b, ] : only 0's may be mixed with negative subscripts

Those :point_up: are the only blocking issues I've got left. I'd also consider using checkmate::assert_integerish() inside .check_wl() to confirm wl is integer-ish (that is, "is a whole number (even if it's stored as a numeric)"), but I don't think it's absolutely necessary. Thanks for the thorough revision!

maRce10 commented 5 months ago

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/609#issuecomment-1904390505

comment: Is your pkgdown website not rendering anymore? The version on the website is out-of-date from your actual Rmd files, and contains a few errors that aren't present in the Rmd.

response: Not sure why github pages is not building the site. Any help with that is welcome

comment: In the alignment vignette, warbleR::is_extended_selection_table(aligned_tests) still returns FALSE for me (when running only user-visible code), while a hidden chunk prints TRUE. Not sure what's going on here -- the fact that this apparently should be an extended selection table, but isn't, feels concerning? But I have no idea if this is actually a problem.

response: I fixed the code. The object class is now evaluated. The class is data frame maRce10/baRulho@1c79390a0b49f01ed8c781d69ac6f989e17b9442

comment:I'm still seeing one error in examples:

>
> # add_noise.R
> data("test_sounds_est")
>
> # make it a 'by element' extended selection table
> X <- warbleR::by_element_est(X = test_sounds_est)
>  |                                                  | 0 % ~calculating  Error in wave[a:b, ] : only 0's may be mixed with negative subscripts
>

response: This was a problem with a dependency, warbleR. Fortunately, I am the maintainer of that package so I updated it on CRAN and udpated the required package version in DESCRIPTION. maRce10/baRulho@d53d24a808f47d901840a13af1aacbd5a2f0c468

comment: I'd also consider using checkmate::assert_integerish() inside .check_wl() to confirm wl is integer-ish (that is, "is a whole number (even if it's stored as a numeric)"), but I don't think it's absolutely necessary.

response: Done maRce10/baRulho@4630f5556668d5a9b7692ede30318a1141f54a87

thanks!

ropensci-review-bot commented 5 months ago

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

@ropensci-review-bot help

jhollist commented 5 months ago

@maRce10 Thanks for submitting your response to @mikemahoney218 feedback.

As a side note, I think the review bot commands need to be submitted in a comment without anything else in that comment. Try that again and see if it works.

@DenaJGibbon could you take a look at the response to your review in https://github.com/ropensci/software-review/issues/609#issuecomment-1889567333 and let us know if there any outstanding concerns?

mikemahoney218 commented 5 months ago

Opened https://github.com/maRce10/baRulho/pull/18 to hopefully fix the website, but otherwise all my concerns have been addressed and I'm comfortable recommending acceptance :smile:

maRce10 commented 5 months ago

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/609#issuecomment-1904390505

ropensci-review-bot commented 5 months ago

Logged author response!

maRce10 commented 5 months ago

Opened maRce10/baRulho#18 to hopefully fix the website, but otherwise all my concerns have been addressed and I'm comfortable recommending acceptance 😄

yes it fixed the website. Thanks a lot!

jhollist commented 5 months ago

@maRce10 quick update. @DenaJGibbon is away doing field work and will respond towards the end of next week.

If you have any questions or concerns, let me know.

DenaJGibbon commented 4 months ago

Hello @maRce10 .

I have had an enjoyable afternoon working through "baRulho" using my own data. I am at the 'excess attenuation' function, and in the vignette it says: "Two methods for calculating excess attenuation are provided (see ‘method’ argument)." However, the function returns an error when I try to specify a method and there is nothing about the method in the function documentation. Could you please clarify? Does this mean based on the two reference methods, or are there two ways to calculate EA with the package?

Below is the output from the 'plot_degradation' function on my own data (gibbons from Borneo). It looks great!

plot_degradation_p1

I am still working through all the functions and will probably not be able to submit the full review until next week. Thanks for your patience!

maRce10 commented 4 months ago

Thanks @DenaJGibbon !

There was an argument in excess_attenuation() to select the type of EA to calculate. However I removed that option as one method was not very reliable. However I forgot to update the vignette. I Just did that in this commit: maRce10/baRulho@69fafb847ae4a012889b0d1f6a117a279651ca4d

DenaJGibbon commented 4 months ago

First, I want to say you have done a great job getting all these functions together in a relatively easy to digest format. This will be a highly useful tool for folks working in bioacoustics. I worked through all of the example using my own data, and have some specific comments based on that experience below. I also apologize for the delay - I have been playing catch up since I got back from the field. I should be able to respond sooner in the future.

Getting set up

I spent much of the time I allotted to do this review getting my data in the correct format. As I suggested in my previous review, I really think that to lower the barrier to entry for folks you should include step-by-step instructions on how to get the data into the correct format. I referred to the 'warbleR' documentation on the extended selection tables, but it was not clear (from reading the documentation) how to get the information about distance, sound.id, etc. into the table. After lots of trial and error I was able to get it working, but for folks that don't have a ton of coding experience this will be hard for them.

Plot degradation

It would be great if you could include bit more information in the description for envelope and spectrum - I dug into the code to see how they were calculated but it should be easier to see. Also if there is some way to include the axis labels or units of the two in the plot (or maybe default printed somewhere) that would be good.

Excess attenuation

I am a bit confused about the example in the EA section, as a proportion of the values are negative (which would indicate amplification of the sound). Could you please double check and let me know what is going on here? I also think that including the equation in the documentation would be helpful here.

Noise profiles

I think there is some missing info: "The function uses internally to calculate frequency spectra. "

jhollist commented 4 months ago

@maRce10 Just pinging to make sure you saw @DenaJGibbon response.

Once you deal with those we can get reviewer concurrence and should have baRulho ready to go!

maRce10 commented 4 months ago

Thanks a lot @DenaJGibbon for your very useful comments! Here are the responses:

comment: I spent much of the time I allotted to do this review getting my data in the correct format. As I suggested in my previous review, I really think that to lower the barrier to entry for folks you should include step-by-step instructions on how to get the data into the correct format. I referred to the 'warbleR' documentation on the extended selection tables, but it was not clear (from reading the documentation) how to get the information about distance, sound.id, etc. into the table. After lots of trial and error I was able to get it working, but for folks that don't have a ton of coding experience this will be hard for them.

_response: You are totally right, this is an issue I have had in mine for a while, mostly related to the package warbleR which is where these different input data formats are defined. baRulho basically take all 3 data formats used by the package warbleR, which I also maintain. So I added a new vignette to warbleR, called "annotation data format" (https://marce10.github.io/warbleR/articles/annotation_data_format.html), to have a more detailed explanation of the structure of these objects. I have added a link to this vignette to the section "Require data structure" in the vignette "Quantify degradation" in baRulho. I have also added definitions for all the required columns in the input data description in "Quantify degradation". maRce10/baRulho@626e999ffa951e69e31fe1b32cfc013f06590273_

comment: Excess attenuation

I am a bit confused about the example in the EA section, as a proportion of the values are negative (which would indicate amplification of the sound). Could you please double check and let me know what is going on here? I also think that including the equation in the documentation would be helpful here.

response: I have revised the code (thanks to input from users) and the EA calculation is now more accurate. Now only 2 measures are negative in the example maRce10/baRulho@176266b43a73bcd01b16b59dc5c42abf26ee75ff. I have also added that " Excess attenuation is computed as -20 * log10(rms("test signal") / rms("reference signal"))) - (20 * log10(1 / "distance") in which 'rms(..)' represents the root mean square of an amplitude envelope." to the function description maRce10/baRulho@35e4406caf0d0e21b75ee7cbd8620c3bc64f4745

comment: Plot degradation

It would be great if you could include bit more information in the description for envelope and spectrum I dug into the code to see how they were calculated but it should be easier to see. Also if there is some way to include the axis labels or units of the two in the plot (or maybe default printed somewhere) that would be good.

response: I have added to the function description that "Amplitude envelopes and power spectra are computed using the functions \code{\link[seewave]{envelope}} (warbleR package) and \code{\link[seewave]{spec}} (seewave package) respectively. This two visualizations show the power distribution in time and frequency between the minimum and maximum power values for each sound. Therefore scales are not necessarily comparable across panels." maRce10/baRulho@e6e441237ce6a04d747a9fdb8c691a56d082d22c

Adding scales would be tricky as plots are already pretty busy. I also think this scales are not extremely informative if sound files are not calibrated as in most cases.

comment: Noise profiles

I think there is some missing info: "The function uses internally to calculate frequency spectra. "

response: fixed! (The function uses \code{\link[seewave]{meanspec}} internally to calculate frequency spectra) maRce10/baRulho@e6e441237ce6a04d747a9fdb8c691a56d082d22c

thanks!

maRce10 commented 4 months ago

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/609#issuecomment-1959822941

ropensci-review-bot commented 4 months ago

Logged author response!

maRce10 commented 3 months ago

@jhollist just wondering if the response to the reviews was noticed 😄

jhollist commented 3 months ago

Thanks for the heads up @maRce10. Not sure why these keep slipping by!

@DenaJGibbon everything look good to you in https://github.com/ropensci/software-review/issues/609#issuecomment-1977695348

DenaJGibbon commented 3 months ago

A few more comments - the package is in pretty good shape! Nice job!

Vignette

Package

jhollist commented 3 months ago

Thank you for these last few comments, @DenaJGibbon. Really great review!

@maRce10 Once these are done, we are good to go and can move on the accept! Let me know if you have questions, and I will make sure to keep my eye on the notifications this time, but also feel free to ping me so I don't let it slip (again...)

maRce10 commented 3 months ago

Thanks again @DenaJGibbon for your useful comments! Here are the responses:

comment: The printout of class(est) appears to have distances in it, but there is not an explanation in the text about where they came from in this vignette (https://marce10.github.io/warbleR/articles/annotation_data_format.html)

response: There was an issue with the ANSI format of the selection table printing method. But it is already fixed maRce10/warbleR@3f3efc3934ddd1902fece4efdca8cfadd69fef9d

comment: I find the printout sections in the vignette page hard to follow/understand. Perhaps another call like head or colnames would provide the info needed and be cleaner?

response: I think this was also fixed with the changes made when dealing with the previous comment

comment: In the 'add_noise' function description it would be helpful to describe what kind of noise (e.g, white noise?)

response: It adds white noise. But I just added a new argument "kind" that can be used for using different (white, pink, brown, red, and power). maRce10/baRulho@e41319953a24aa3e404fbe83b3c47b1497a981b6

comment: The 'detection_distance' function is in the package, but I couldn't find anything in the vignette about it (unless am I missing something?). This is a cool feature, but I couldn't find much info on it. And when I run the example the output is not intuitive.

response: I added this to the function description: "The function computes the maximum distance at which a sound would be detected, which is calculated as the distance in which the sound pressure level (SPL) goes below the specified SPL cutoff ('spl.cutoff')). This is returned as an additional column 'detection.distance' (in m). " maRce10/baRulho@e6b07b137ae3c56913357b71ed31181e6878f9e9

comment: I ran through all the (~22) function examples, and they all worked - nice job!

_response: sweeet!

thanks!

(@jhollist)

jhollist commented 3 months ago

@maRce10, @DenaJGibbon, and @mikemahoney218 I think we are there! Thank you all for you efforts on this.

jhollist commented 3 months ago

@ropensci-review-bot approve baRulho

ropensci-review-bot commented 3 months ago

Approved! Thanks @maRce10 for submitting and @mikemahoney218, @DenaJGibbon for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. They will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

maRce10 commented 3 months ago

@ropensci-review-bot finalize transfer of baRulho

ropensci-review-bot commented 3 months ago

Transfer completed. The baRulho team is now owner of the repository and the author has been invited to the team

maRce10 commented 3 months ago

@DenaJGibbon @mikemahoney218 would you like to be included in the package as contributors?

mikemahoney218 commented 3 months ago

If you see fit, I'd happy to be included as role ="rev"!

DenaJGibbon commented 2 months ago

Sure, thanks! Nice job on getting this out there!