ropensci/software-review

pathviewR: Tools to import, clean, and visualize animal movement data in R

vbaliga opened this issue · 34 comments

vbaliga commented

Submitting Author: Name (@vbaliga)
Repository: https://github.com/vbaliga/pathviewr
Version submitted: v0.9.4 (ropensci/pathviewr@6e04870)
Editor: @maelle
Reviewers: @asbonnetlebrun, @marcosci

Due date for @asbonnetlebrun: 2020-12-05

Due date for @marcosci: 2020-12-21
Archive: TBD
Version accepted: TBD

  • Paste the full DESCRIPTION file inside a code block below:
Package: pathviewR
Title: Wrangle, Analyze, and Visualize Animal Movement Data
Version: 0.9.4
Authors@R: 
    c(person(given = "Melissa S.", 
             family = "Armstrong",
             role = "aut",
             email = "melissa.armstrong@gmail.com",
             comment = c(ORCID = "0000-0002-3059-0094")),
      person(given = "Vikram B.",
             family = "Baliga", 
             role = c("aut", "cre"),
             email = "vbaliga87@gmail.com",
             comment = c(ORCID = "0000-0002-9367-8974")),
      person(given = "Eric R.", 
             family = "Press",
             role = "aut",
             email = "epress12@gmail.com",
             comment = c(ORCID = "0000-0002-1944-3755"))
      )
Description: Tools to import, clean, and visualize 
    animal movement data from motion capture systems such as Optitrack's 
    Motive, the Straw Lab's Flydra, or from other sources. We provide 
    functions to remove artifacts, standardize tunnel position and tunnel 
    axes, select a region of interest, isolate specific trajectories, fill
    gaps in trajectory data, and calculate 3D and per-axis velocity. For 
    experiments of visual guidance, we also provide functions that use 
    animal position to estimate perception of visual stimuli. 
License: GPL-3
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1.9000
Imports: 
    R.matlab,
    data.table (>= 1.12.2),
    magrittr (>= 1.5),
    dplyr (>= 1.0.0),
    stringr (>= 1.4.0),
    tibble (>= 3.0.1),
    tidyr (>= 1.1.0),
    fANCOVA,
    purrr (>= 0.3.3),
    ggplot2 (>= 3.3.0),
    tidyselect (>= 1.1.0),
    cowplot
Suggests: 
    knitr,
    rmarkdown,
    testthat,
    covr
VignetteBuilder: knitr
URL: https://github.com/vbaliga/pathviewR
BugReports: https://github.com/vbaliga/pathviewR/issues

Scope

  • Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

    • data retrieval
    • data extraction
    • data munging
    • data deposition
    • workflow automation
    • version control
    • citation management and bibliometrics
    • scientific software wrappers
    • field and lab reproducibility tools
    • database software bindings
    • geospatial data
    • text analysis

  • Explain how and why the package falls under these categories (briefly, 1-2 sentences):
    pathviewR provides tools to extract data from motion capture software such as Optitrack's Motive and the Straw Lab's Flydra (among other sources). Tools to wrangle and clean data sets to isolate specific trajectories of animal movement are offered, along with the capability to use automated workflows for processing data from high throughput laboratory experiments.

  • Who is the target audience and what are scientific applications of this package?
    Users of motion capture software, including researchers who study animal and/or object motion in a laboratory setting, especially for analyses of visual guidance of behavior.

  • Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
    We are not aware of any other R (or otherwise open source) packages that import & wrangle data from Optitrack's Motive or the Straw Lab's Flydra. Although there are other R packages that handle animal movement trajectory data, these packages tend to be geared towards analyses of large-scale, geospatial movement data (e.g. trajectories or various R packages in the AniMove software suite). In contrast, pathviewR specifically targets tracking of animals (or other subjects) in laboratory settings (e.g. in a flight tunnel) and offers tools to estimate visual perceptions as subjects move in relation to their surroundings.

  • (If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
    pathviewR, although not specifically geared towards collecting data from human subjects, is in compliance with rOpenSci's policy.

  • If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you intend for this package to go on Bioconductor?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
JOSS Options
  • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)
MEE Options
  • The package is novel and will be of interest to the broad readership of the journal.
  • The manuscript describing the package is no longer than 3000 words.
  • 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)
  • (Scope: Do consider MEE's Aims and Scope 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

maelle commented

Thanks @vbaliga and co-authors for your submission! I have a few minor comments/questions below, and will now start looking for reviewers.

Editor checks:

  • Documentation: The package has sufficient documentation available online (README, pkgdown docs) to allow for an assessment of functionality and scope without installing the package.
  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly

Editor comments

Reviewers:
Due date:

maelle commented

Please add this badge to the package README

[![](https://badges.ropensci.org/409_status.svg)](https://github.com/ropensci/software-review/issues/409)
vbaliga commented

Hi @maelle,

Thank you for looking over pathviewR and providing us with feedback. My co-authors (@scienceisfiction and @epress12) and I worked together earlier today to address each of your comments, and a few changes to our repo have been made in ropensci/pathviewr@688c198. Here is our reply to each of your specific comments:

In https://vbaliga.github.io/pathviewR/articles/data-import-cleaning.html#data-from-other-sources-1 I would expect to read whether you'd welcome additions of further data import functions, and how an user should request/contribute such a function.

Thanks for pointing this out. We very much welcome users to reach out to us regarding further import functions. We created an Issue template in our repository to allow users to request further import functions, and a link to our Issues templates is now provided in the section of the vignette that you specified.

Why is the CITATION TBD, why not add a citation of the manual for now?

Thanks for catching this! We have now added a citation of the Manual of this package. It appears both as a CITATION file within /inst as well as in the Citation section of our readme.

Why do you use so many continuous integration services, since GitHub Actions would handle all operating systems?

Please forgive our ignorance. We use Travis CI because I had the impression that it was emphasized in the rOpenSci devguide and because of my familiarity with it in authoring a previous rOpenSci package (ropensci/workloopR). We added GitHub Actions because we were interested in seeing what it had to offer. Admittedly, we are less familiar with GitHub Actions' breadth of coverage (moreover, it is a newer service, right?). Would you advise we use only GitHub Actions? Or is it okay that we continue using both Travis CI and GitHub Actions?

Please add this badge to the package README

The badge has been added. Thanks again for considering our package!

Best regards,
Vikram
🐢

maelle commented

👋 @vbaliga, @scienceisfiction and @epress12! Thanks for your work!

  • Regarding the issue template, awesome! Would you also welcome people to contribute functions and if so what would be the process? Maybe that could be a question in the template?

  • Regarding continuous integration yes that's hard to keep up. In https://devguide.ropensci.org/ci.html#whichci we've now put GitHub Actions first which is admittedly not a very obvious way to recommend it. We recommend it over Travis now because it has all operating systems, and because Travis is changing its pricing. E.g. https://www.jeffgeerling.com/blog/2020/travis-cis-new-pricing-plan-threw-wrench-my-open-source-works And since GitHub Actions has support for Windows, you can do without Appveyor. Hard to tell what we'll all be using in a few years 😅

I've found a reviewer that I'll assign soon, and am still looking for the second reviewer.

maelle commented

Thanks a lot @asbonnetlebrun for agreeing to review! Your review is due on 2020-12-09.

Please find links to our reviewer guide and review template.

vbaliga commented

Hi @maelle,

Thanks very much for your advice. We've expanded our issue template to welcome people to contribute functions and added a question to the template accordingly (see ropensci/pathviewr@7c4c32d). We have also now shifted to using GitHub Actions exclusively for CI using the "standard" check across Mac/Win/Linux and revised our method of updating Codecov through Actions as well (see ropensci/pathviewr@f3a787b).

Thanks again!
Vikram
🐢

  • 🚀1
maelle commented

Thanks a lot @marcosci for agreeing to review! Your review is due on 2020-12-21.

Please find links to our reviewer guide and review template.

  • 👍1
maelle commented

With this, now both reviewers @asbonnetlebrun and @marcosci are assigned. 😸

  • 🎉1
stefaniebutland commented

Hi @asbonnetlebrun. I'm rOpenSci's Community Manager. If you would like to join our Slack group, please email me at stefanie@ropensci.org; I was not able to find your email address.

asbonnetlebrun commented

Hi @maelle and @vbaliga,
I will likely be week late for handing in my review - very sorry for the delay!

maelle commented

Thanks for the update @asbonnetlebrun!

asbonnetlebrun commented

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • Briefly describe any working relationship you have (had) with the package authors.

I don’t have any working relationship with any of the package authors.

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally

There are examples for almost all exported functions in R help. Exceptions are the calc_sf_box(), calc_sf_V(), calc_vis_angle_box() and calc_vis_angle_V() functions (and standardised_tunnel, although there is a reason given in the R help – because no example data is provided with pathviewR to run the code on).

  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Only in the DESCRIPTION file, but not in the README.

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.

See details in my comments about some potential coding mistakes.

  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.

All tests pass on my machine as they have been designed (but see in my comments for one test that likely should be re-written).

  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 8

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

I want to first point out that my background is on animal movement data, but not with motion capture cameras (I focus on tracking of wild animals, using GPS or similar loggers), so I was not fully familiar with the interests/culture of the field.

I thought the package was well documented overall, and the three vignettes in particular really helped me understand what the package was for. That being said, I think it would still benefit from some additional information in the different documents (vignettes, README, R help). See my comments below from some specific points that would be good to add.

I also very much appreciated the effort made to make it all work with pipes.

In the rest of my review, I’ll first focus on comments on the three vignettes to help clarify some points to the users. Then I’ll focus on the code of the visual perception functions, where I think I found some mistakes and that would be worth checking.

Data import and cleaning
My understanding is that the “Relabeling axes” and “gathering data columns” parts are only relevant in the case of Motive data (Flydra data already comes in the correct format, and as_viewr(), the function to import other types of data also takes data in the correct format and already relabels the columns). Maybe this could be made a bit clearer in the vignette?

Maybe that was me not being very familiar with the kind of experiments the package is relevant for, but I had trouble to understand when we would use the standardize_tunnel() function or the rotate_tunnel() function. Maybe you could provide some examples when these functions to be applied? Also I struggled to understand the standardize_tunnel() function, maybe particularly because there was no example in the help file. When would the landmarks already be in the data? I guess it will never be the case with Flydra data, considering that the read_flydra_mat() function allows to input only a single subject_name. Is that why it doesn’t need to be rotated, or is that something arising from the Flydra software?

Also, considering how the select_x_percent() function works (by selecting a certain percentage of the tunnel length on each site of (0,0,0) along the length axis), shouldn’t it be more appropriate to say that the (0,0,0) must be at the centre of the region of interest, rather than at the centre of the tunnel?

Minor point: the link to the vignette for managing frame gaps is missing in the text.

Managing frame gaps
Very minor point: could it be clearer in the visualize_frame_gap_choice() help or in the vignette that the loops argument represents the maximum frame gap considered (i.e. that each loop represents an increment of 1 on the frame gap value)?

Could there be cases when frame gaps can vary between devices (i.e. if I understood well in the case of the Motive data, between subjects)? If so, would it be relevant to allow the autodetect approach to be applied to each subject separately? (or maybe that is not relevant...)

Visual perception
This is where I struggled, but maybe that is because I don’t come from the specific field of application of this package. I felt like some more details could be included in the vignette. Maybe start with a few examples of experiments where this package can be useful. Maybe you could say right at the start that these functions are relevant for experiments with animals flying in tunnels with visual stimuli consisting of sine wave gratings on the tunnel walls?

Also, my understanding is that here you implement only two cases:

  • Motive example: birds flying through a V-shaped tunnel, with visual stimuli on each side of the tunnel
  • Flydra example: birds flying through a rectangular tunnel, with visual stimuli on the side and front walls
    So maybe make it clear that this section only applies to these two specific cases?

On this, a question: is there any plan to code more situations or are these the typical situations for these kinds of experiment? If you welcome suggestions from users of other settings, maybe you could say that in the vignette in the same way you mention that you are happy to work towards the inclusion of more data types?

More generally, I would have appreciated some definition of what you call “spatial frequency” and “visual angle”. Although I understand that these might be obvious for people in the field (which are ultimately the target users of the package), but I feel like these terms (visual angle, and in particular spatial frequency) are of such broad usage that they might deserve some better description of what they mean in the context of the package. Maybe include a diagram in the vignette for users to visualise what these values represent?

In particular, the Value section in the R help for the calc_sf_box() function seems to mention that the spatial frequency is the number of cycles per degree of visual angle. Maybe this info could be included in the vignette (and in the Description section of the function help)?

Comments on the code of the visual perception functions
I seem to understand that the functions to calculate visual angle and spatial frequency in the box example don’t handle the front wall (only the side walls), am I right? Is there any reason for this? If the front wall is not relevant, maybe there is no need to include the option to add parameters for it in the insert_treatments() function?

My understanding of the visual angle is as follows: let’s consider an isosceles triangle with one vertex at the bird’s eye, with the side of unique length being on the wall and of a length equal to the wavelength of the stimulus, and the third vertex to be at the bird’s eye, with the bisector of the angle at the bird’s eye being the segment of shortest distance between the bird and the wall (would have been easier with a diagram, sorry). Then the angle at the bird’s eye should be the visual angle – right?

If I am correct, let’s dig into the calc_sf_box(), calc_sf_V(), calc_vis_angle_box() and calc_vis_angle_V() functions.
My understanding in all these functions is that min_dist_pos is the distance between the bird and the positive wall, and min_dist_neg, between the bird and the negative wall.
Calculations for these values are redundant between calc_sf_box() and calc_vis_angle_box(), as well as between calc_sf_V() and calc_vis_angle_V(). I would recommend to write functions to calculate these distances, that would then be called in the calc_* functions to ensure consistency. In particular, currently the min_dist_neg calculations differ between calc_sf_box() and calc_vis_angle_box(), the correct one being in calc_sf_box(). The effect of the error in calc_vis_angle_box() is that once the widths become positive, the distance to the negative wall start to decrease the further away we get from this wall. This can be visualised by slightly tweaking the final bit of code in the “Visual perception” vignette:

ggplot(flydra_box_angle, aes(x = position_width, y = position_length)) +
  geom_point(aes(colour = min_dist_neg), size = 2) +
  scale_colour_viridis_c() +
  coord_fixed() +
  geom_segment(aes(x = -0.5,        # negative wall
                   y = -0.5,
                   xend = -0.5,
                   yend = 0.5)) +
  geom_segment(aes(x = 0.5,         # positive wall
                   y = -0.5,
                   xend = 0.5,
                   yend = 0.5))

image

On that note, there is no need for an ifelse() to calculate these distances, these lines could simply replace these lines and these lines in calc_sf_box() and calc_vis_angle_box():

  obj_name$min_dist_pos <- obj_name$pos_wall - obj_name$position_width
  obj_name$min_dist_neg <- abs(obj_name$neg_wall + obj_name$position_width)

Can you please correct the calculations, and adapt the test-calc_vis_angle_box.R file?

And also on this, I noticed that the user can supply negative values for the neg_wall and pos_wall arguments in the insert_treatments() function, which in this case would lead to spurious distances being calculated. Maybe insert a warning/error message if that is the case, and add the appropriate tests?

Finally, still on distances, inspecting the resulting distances to the negative and positive walls from a range of positive and negative width positions seem to indicate that the distance calculations for the other functions are correct.

Spatial frequency
Another point, on the spatial frequency. If I understand it well (being the number of cycles per degree of visual angle), I think there is an error in these lines of calc_sf_V() and these lines of calc_sf_box().

It currently is:

deg_dist_pos <- 2 * obj_name$min_dist_pos * tan(deg_2_rad(1))
deg_dist_neg <- 2 * obj_name$min_dist_neg * tan(deg_2_rad(1))

I think, unless I got the trigonometry wrong, that this represents the number of cycles per 2 degrees of visual angle, and therefore should be changed to:

deg_dist_pos <- 2 * obj_name$min_dist_pos * tan(deg_2_rad(1/2))
deg_dist_neg <- 2 * obj_name$min_dist_neg * tan(deg_2_rad(1/2))

Just one final broader question, don’t these calculations assume that the bird is parallel to the length axis of the tunnel? Is that not important/common practice not to use the rotation information?

Many thanks for your contribution, and I hope this will helps!

  • 👍2
maelle commented

Many thanks @asbonnetlebrun for your thorough review! 🙏

vbaliga commented

@asbonnetlebrun, thank you very much for your thoughtful & constructive review! My co-authors and I have each read through your comments and appreciate the time & effort you put in to reviewing our package

@maelle, this is a slightly awkward time for us -- our university's semester is wrapping up and of course the holiday season is approaching. That said, I am certain we can address our reviews in a timely manner. Since we are also pending one more review, would it be OK if we held off on making changes until both reviews are in? Given that the due date for @marcosci's is 2020-12-21, I am confident that my co-authors and I could address both reviews within a couple weeks after (perhaps by 2021-01-04?). Would that be OK with you?

maelle commented

@vbaliga definitely ok! Besides, not making changes on the default branch right now will prevent @marcosci from reviewing a moving target. 🙂

  • 👍1
marcosci commented

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • Briefly describe any working relationship you have (had) with the package authors.
  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 5

  • Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Following @asbonnetlebrun example - I am coming from a landscape ecology, so looking at your package from a system detached point of view. I tried the package out with a .csv file from my phone that I tracked while going for a walk and everything worked just fine.

Overall, there is really not much one can add to @asbonnetlebrun comments. The package is in an exceptional shape, there are:

  • tests for everything,
  • no spellings,
  • no notes, warnings, errors.

All examples in the vignettes run just fine. Furthermore, the package itself is well documented,
so every function in there has at least some comments. I appreciated that a lot!

I will list a couple of things that crossed my mind:

  • In the description etc. you state that pathviewR is a tool for "animal movement data" ... Couldn't you make that broader? Part of the package is actually capable to handle all kinds of 3D movement data and the visual perception function should also be applicable to humans, or?
  • It would actually be quite beneficial to give a short walkthrough of what input data can look like. So, you need x,y,z ... but what more. And what defines Optitrack and flydra data.
  • I did not see any contribution guidelines, so it would be helpful to include those.
  • The Maintainer field is missing in the DESCRIPTION - altough Vikram is listed as CRE.
  • pathviewR ... I submitted a package here once and the feedback was to either stick with capital letters or just lower case. I don't know if that changed, but it makes typing the package name out easier in my opinion. If that is a must should probably be addressed by @maelle.

Other than that ... impressive work! The package is in excellent good shape and is in my opinion a nice addition to the rOpenSci bestiary.

maelle commented

Thanks a ton @marcosci for your review! 😸

I tried the package out with a .csv file from my phone that I tracked while going for a walk and everything worked just fine.

Such a neat idea!

Regarding the package name, yes we recommend all lower-case, especially if the package isn't on CRAN yet. I myself renamed my Ropenaq package after review and I don't regret doing it.

  • 👍1
vbaliga commented

Hi everyone -- hope you have been enjoying the holiday season!

@marcosci -- thank you very much for your constructive review

@maelle -- thank you for your advice; we are considering renaming the package. We'll likely do this as a final step after addressing all the reviewers' other comments.

@asbonnetlebrun and @marcosci -- we would be happy to list you both as "rev" in our description file. Could you please let us know how you'd like your names to appear and what other contact info (email, ORCID...etc) you wish to have listed?

As you may have seen, we have been working on addressing all the comments. We'll let you know when we have rounded out all our edits.

Thanks!
Vikram
🐢

maelle commented

Happy New Year @vbaliga, @scienceisfiction and @epress12, and thanks for the update! (also Happy New Year to @asbonnetlebrun & @marcosci)

vbaliga commented

Hi @asbonnetlebrun, @marcosci, and @maelle,

Thank you sincerely for taking the time and effort to review our submission.

Below, you will find point-by-point responses to each of your comments. Your original comments may not appear fully (for sake of concision), but please note that we believe that we have addressed each item to the best of our capabilities.

Since addressing these items was handled over several commits, the most appropriate commit that reflects the change will also be noted within each of our responses.

Please also note that the package is now entitled pathviewr instead of the original pathviewR, per your advice.

Review from Reviewer 1 (asbonnetlebrun)

Documentation

Examples for all exported functions in R Help that run successfully locally

There are examples for almost all exported functions in R help. Exceptions are
the calc_sf_box(), calc_sf_V(), calc_vis_angle_box() and calc_vis_angle_V()
functions (and standardised_tunnel, although there is a reason given in the R
help – because no example data is provided with pathviewr to run the code on).

We added in examples for each of the following functions. Please note that we consoldiated some of the visual guidance functions, so some of the original functions referred to in this reviewer comment have now been replaced or renamed.

Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Only in the DESCRIPTION file, but not in the README.

We have added contribution guidelines (with links to Issues pages) in our README. (f5d47e7)

Functionality

We will address these items in the Review Comments section below

Should the author(s) deem it appropriate, I agree to be acknowledged as a
package reviewer ("rev" role) in the package DESCRIPTION file.

We have added you as a "rev" within our description file (5b0661). Please let us know if you would like your name to appear differently and/or would like to adjust contact info etc.

Review Comments

Data import and cleaning

My understanding is that the “Relabeling axes” and “gathering data columns”
parts are only relevant in the case of Motive data (Flydra data already comes in
the correct format, and as_viewr(), the function to import other types of data
also takes data in the correct format and already relabels the columns). Maybe
this could be made a bit clearer in the vignette?

We have clarified language of this vignette to indicate that relabeling & gathering are only necessary in certain cases (e.g. using Motive data). Please let us know what you think. (591dd50)

Maybe that was me not being very familiar with the kind of experiments the
package is relevant for, but I had trouble to understand when we would use the
standardize_tunnel() function or the rotate_tunnel() function. Maybe you could
provide some examples when these functions to be applied? Also I struggled to
understand the standardize_tunnel() function, maybe particularly because there
was no example in the help file. When would the landmarks already be in the
data? I guess it will never be the case with Flydra data, considering that the
read_flydra_mat() function allows to input only a single subject_name. Is that
why it doesn’t need to be rotated, or is that something arising from the Flydra
software?

We have clarified the circumstances under which standardization is needed and what types of landmarks are appropriate (591dd50). Additionally, in the Help file for standardize_tunnel(), we have clarified this function's use cases (591dd50)

Also, considering how the select_x_percent() function works (by selecting a
certain percentage of the tunnel length on each site of (0,0,0) along the length
axis), shouldn’t it be more appropriate to say that the (0,0,0) must be at the
centre of the region of interest, rather than at the centre of the tunnel?

Thanks! We have revised language of this vignette on what (0,0,0) represents (591dd50)

Minor point: the link to the vignette for managing frame gaps is missing in
the text.

Thanks for catching this -- we have added the link (591dd50)

Managing frame gaps

Very minor point: could it be clearer in the visualize_frame_gap_choice() help
or in the vignette that the loops argument represents the maximum frame gap
considered (i.e. that each loop represents an increment of 1 on the frame gap
value)?

We have added details of what loops means to both the vignette (bcf6424) and to the Help file (bcf6424)

Could there be cases when frame gaps can vary between devices (i.e. if I
understood well in the case of the Motive data, between subjects)? If so, would
it be relevant to allow the autodetect approach to be applied to each subject
separately? (or maybe that is not relevant...)

Sorry for being unclear - this function actually has this feature! We have made a slight modification to the language that will hopefully make it more clear (62b63b7)

Visual perception

This is where I struggled, but maybe that is because I don’t come from the
specific field of application of this package. I felt like some more details
could be included in the vignette. Maybe start with a few examples of
experiments where this package can be useful. Maybe you could say right at the
start that these functions are relevant for experiments with animals flying in
tunnels with visual stimuli consisting of sine wave gratings on the tunnel
walls?

Thanks. We have heavily revised the language of the first couple paragraphs accordingly, and a figure has also been inserted to hopefully give readers a better sense of the concepts at hand. Please let us know what you think! (97704bc)

Also, my understanding is that here you implement only two cases:

Motive example: birds flying through a V-shaped tunnel, with visual stimuli
on each side of the tunnel
Flydra example: birds flying through a rectangular tunnel, with visual
stimuli on the side and front walls. So maybe make it clear that this section
only applies to these two specific cases?

On this, a question: is there any plan to code more situations or are these
the typical situations for these kinds of experiment? If you welcome suggestions
from users of other settings, maybe you could say that in the vignette in the
same way you mention that you are happy to work towards the inclusion of more
data types?

We have revised the language of how these experiments are introduced (97704bc). We have also provided links to the Issues page where users can request e.g. different tunnel setups (20bb54a)

More generally, I would have appreciated some definition of what you call
“spatial frequency” and “visual angle”. Although I understand that these might
be obvious for people in the field (which are ultimately the target users of the
package), but I feel like these terms (visual angle, and in particular spatial
frequency) are of such broad usage that they might deserve some better
description of what they mean in the context of the package. Maybe include a
diagram in the vignette for users to visualise what these values represent? In
particular, the Value section in the R help for the calc_sf_box() function
seems to mention that the spatial frequency is the number of cycles per degree
of visual angle. Maybe this info could be included in the vignette (and in the
Description section of the function help)?

We have added in definitions along with citations of some journal articles that provide nice summaries of these topics. (fa7b12e)

Comments on the code of the visual perception functions

I seem to understand that the functions to calculate visual angle and spatial
frequency in the box example don’t handle the front wall (only the side walls),
am I right? Is there any reason for this? If the front wall is not relevant,
maybe there is no need to include the option to add parameters for it in the
insert_treatments() function?

Thanks for the suggestion. The visual perception functions now include calculations for the end walls, though the outputs from these functions only include the end wall towards which the subject is moving towards. Please see the changes to following functions:

Note from VBB: for brevity, the details of your calculations will not be included here.

On that note, there is no need for an ifelse() to calculate these distances,
these lines could simply replace these lines and these lines in calc_sf_box()
and calc_vis_angle_box()

Thanks! We have now replaced the ifelse() lines in the corresponding functions, which also have been renamed:

Can you please correct the calculations, and adapt the
test-calc_vis_angle_box.R file?

Thanks sincerely for catching this! We have made corrections to the calculations of vis angle and SF (6a36758)(99c9ef5).

We have also updated test-get_vis_angle.R accordingly Tests were written for all new functions (calc_min_dist_v()(93808ea), calc_min_dist_box(93808ea), get_vis_angle()(ba8d813), and get_sf())(b4afca9).

And also on this, I noticed that the user can supply negative values for the
neg_wall and pos_wall arguments in the insert_treatments() function, which in
this case would lead to spurious distances being calculated. Maybe insert a
warning/error message if that is the case, and add the appropriate tests?

Great catch! We have now added guidance to the Help file of insert_treatments() about feasible values for all arguments including new tunnel dimensions arguments tunnel_width and tunnel_height(429258b). This includes a check within insert_treatments() that stops the function if faulty argument values are supplied (429258b). A test to test-insert_treatments.R to check the functionality of the
above check has also been added (a9cc804)

Spatial frequency

Another point, on the spatial frequency. If I understand it well (being the
number of cycles per degree of visual angle), I think there is an error in these
lines of calc_sf_V() and these lines of calc_sf_box().

Thanks, we have made corrections to the calculation of SF (99c9ef5)

Just one final broader question, don’t these calculations assume that the bird
is parallel to the length axis of the tunnel? Is that not important/common
practice not to use the rotation information?

Yes, this is true and a great point. It is actually not common practice in the field to use rotation information -- quite a few studies still rely solely on positional data. That said, we definitely agree that adding rotation is an important way to advance our understanding of visual guidance. At this time, we allow for rotation data to be imported & wrangled along with all the positional data, but rotation data have not yet been integrated into the visual perception functions. This is largely because how rotation data are encoded can be tricky (depending on how the rotation matrix is composed and/or how orientation axes are defined on each subject). Accordingly, we are still working on a way that will work generically for most use cases.

For now, we have added a note in the Help files for each visual perception that rotation information may be integrated in future pathviewr updates (46ba850). We have also done so in the Visual perception vignette (a3a7795)

Review from Reviewer 2 (marcosci)

Should the author(s) deem it appropriate, I agree to be acknowledged as a
package reviewer ("rev" role) in the package DESCRIPTION file.

We have added you as a "rev" within our description file (5b0661). Please let us know if you would like your name to appear differently and/or would like to adjust contact info etc.

Review Comments

In the description etc. you state that pathviewr is a tool for "animal
movement data" ... Couldn't you make that broader? Part of the package is
actually capable to handle all kinds of 3D movement data and the visual
perception function should also be applicable to humans, or?

Yes thanks, this is a good point and we agree. We have made a slight alteration to our description and readme. We don't want to oversell the features of our package, so we hope that where we have landed with the language strikes a nice
balance. (deffd9d)

It would actually be quite beneficial to give a short walkthrough of what
input data can look like. So, you need x,y,z ... but what more. And what defines
Optitrack and flydra data.

We agree and added a short walkthrough of what movement data look like, both generally and specifically in Motive and Flydra (591dd50)

I did not see any contribution guidelines, so it would be helpful to include
those.

We have added contribution guidelines provided by rOpenSci (with slight modification) (5252918)

The Maintainer field is missing in the DESCRIPTION - altough Vikram is listed
as CRE.

We've updated the Maintainer field with VBB as the maintainer (9146e09)

pathviewR ... I submitted a package here once and the feedback was to either
stick with capital letters or just lower case. I don't know if that changed, but
it makes typing the package name out easier in my opinion. If that is a must
should probably be addressed by @maelle.

Yeah, we see what you mean and went ahead and changed the name to pathviewr. (52e559c)

Additional items from the editor (maelle)

Regarding the package name, yes we recommend all lower-case, especially if the
package isn’t on CRAN yet. I myself renamed my Ropenaq package after review and
I don’t regret doing it.

As noted above, we changed the name to pathviewr. (52e559c). In particular, we found this guide to be very helpful -- we went step-by-step through everything in that page. We figured it would be good to let you know in case you are interested in providing that sort of info in the rOpenSci Guide for Authors, though we understand it may not be common enough of a problem to merit adding to the guide.

On behalf of @scienceisfiction and @epress12, thanks again for all your feedback and advice!

And sorry for the slight delay in getting back to you. My (VBB's) wife just gave birth to our first child last week -- it's been a pretty wild ride!

Best regards,
Vikram
🐢

maelle commented

@vbaliga wow, congratulations! 🥚 🐢 ✨

@vbaliga @scienceisfiction and @epress12, thanks a lot for your work and detailed response!
The dev guide has a link to Nick's post https://devguide.ropensci.org/building.html#package-name-and-metadata, but I should have reminded you of its existence!

@asbonnetlebrun @marcosci Could you please indicate whether you are happy with the authors' response? Template https://devguide.ropensci.org/approval2template.html

maelle commented

@asbonnetlebrun @marcosci Friendly reminder 😸 Could you please indicate whether you are happy with the authors' response? Template devguide.ropensci.org/approval2template.html

asbonnetlebrun commented

@maelle @vbaliga Really sorry for the delay, I have to meet several deadlines this month and have not yet had time to look at the authors' response. I will be more available after the end of next week, and will look at the responses then.

  • 👍2
asbonnetlebrun commented

@maelle @vbaliga Sorry again for taking so long to have a look. I've now checked the authors' response and I am very happy with it. In particular, the authors made a really nice job at clarifying what the different functions of the package are for (very nice figure in the visual perception vignette!). Great job, thanks!

  • 🎉2
marcosci commented

Reviewer Response

Sorry - I was quite buried in things the last couple of weeks 😮
The authors made a great job replying to everything we pointed and I think the package is in a really good shape now (as it was already before the review in great parts).

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: I may have forgotten how much time I spent reviewing ...

  • 🎉1
ropensci-review-bot commented

Approved! Thanks @vbaliga for submitting and @asbonnetlebrun, @marcosci for your reviews! 😁

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
  • Fix all links to the GitHub repo to point to the repo under the ropensci organization.
  • Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
  • If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
    • deactivate the automatic deployment you might have set up
    • remove styling tweaks from your pkgdown config but keep that config file
    • replace the whole current pkgdown website with a redirecting page
    • replace your package docs URL with https://docs.ropensci.org/package_name
    • In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname). If Appveyor does not pick up new commits after transfer, you might need to delete and re-create the Appveyor project. (Repo transfers are smoother with GitHub Actions)
  • Please check you updated the package version post-review version updated and that you documented all changes in NEWS.md
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

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). More info on this here.

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 @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

  • 🎉1
maelle commented

(don't mind me testing the bot now works correctly 😅 )

vbaliga commented

Hi everyone - thanks so much for reviewing pathviewr and for all your feedback. We're excited to have this be part of the ropensci org. I have now begun the transfer of the repo and post-review updating

Cheers,
Vikram
🐢

  • 🎉1
maelle commented

I can now ask one last question: why the turtle? 🙂

vbaliga commented

Lots of little reasons, but among them one of the bigger ones is: https://xkcd.com/889/
🐢

  • 👍1
  • 😄1
vbaliga commented

Hi @maelle, thanks again for all your help. We have taken care of all the items on the to-do list.

To-dos:

  • Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do. 
  * deactivate the automatic deployment you might have set up
  * remove styling tweaks from your pkgdown config but keep that config file
  * replace the whole current `pkgdown` website with a [redirecting page](https://devguide.ropensci.org/redirect.html)
  * replace your package docs URL with `https://docs.ropensci.org/package_name`
  * In addition, in your DESCRIPTION file, include the docs link in the `URL` field alongside the link to the GitHub repository, e.g.: `URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar`
  • Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname). If Appveyor does not pick up new commits after transfer, you might need to delete and re-create the Appveyor project. (Repo transfers are smoother with GitHub Actions)
  • Please check you updated the package version post-review version updated and that you documented all changes in NEWS.md
  • We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

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). More info on this here.

Happy to! Thanks again for the reviews.

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 @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

Thanks, we are discussing the idea of this and will be sure to reach out to Stefanie if we opt to contribute

maelle commented

Awesome, thanks for the update! Please also tell me if other authors need to be invited to the ropensci GitHub organization.