search

ropensci / software-review

rOpenSci Software Peer Review.
287 stars 104 forks source link

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

Closed vbaliga closed 3 years ago

vbaliga commented 3 years ago

Submitting Author: Name (@vbaliga)
Repository: https://github.com/vbaliga/pathviewr Version submitted: v0.9.4 (https://github.com/vbaliga/pathviewr/commit/6e04870d7451b05370255a6d0c7d6532f4cc1e5a) Editor: !--editor-->@maelle<!--end-editor-- Reviewers: @asbonnetlebrun, @marcosci

Due date for @asbonnetlebrun: 2020-12-05 Due date for @marcosci: 2020-12-21

Archive: TBD
Version accepted: TBD 

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

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

JOSS Options - [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) 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](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

maelle commented 3 years ago

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:

Editor comments

Reviewers: Due date:

maelle commented 3 years ago

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 3 years ago

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 https://github.com/vbaliga/pathviewR/commit/688c198466d1a88a7ff07099263369f6fa39b06c. 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 3 years ago

:wave: @vbaliga, @scienceisfiction and @epress12! Thanks for your work!

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

maelle commented 3 years ago

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 3 years ago

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 https://github.com/vbaliga/pathviewR/commit/7c4c32d977baf966da76366c88e746630cf17f43). 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 https://github.com/vbaliga/pathviewR/commit/f3a787b4350c12e8ec707e3e0a4cef9b6a7dca6f).

Thanks again! Vikram 🐢

maelle commented 3 years ago

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.

maelle commented 3 years ago

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

stefaniebutland commented 3 years ago

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 3 years ago

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

maelle commented 3 years ago

Thanks for the update @asbonnetlebrun!

asbonnetlebrun commented 3 years ago

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

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

Documentation

The package includes all the following forms of documentation:

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).

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

Functionality

See details in my comments about some potential coding mistakes.

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).

Estimated hours spent reviewing: 8

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:

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!

maelle commented 3 years ago

Many thanks @asbonnetlebrun for your thorough review! :pray:

vbaliga commented 3 years ago

@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 3 years ago

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

marcosci commented 3 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 5

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:

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:

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 3 years ago

Thanks a ton @marcosci for your review! :smile_cat:

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.

vbaliga commented 3 years ago

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 3 years ago

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

vbaliga commented 3 years ago

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 3 years ago

@vbaliga wow, congratulations! :egg: :turtle: :sparkles:

@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 3 years ago

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

asbonnetlebrun commented 3 years ago

@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.

asbonnetlebrun commented 3 years ago

@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!

marcosci commented 3 years ago

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)

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

maelle commented 3 years ago

Thank you @asbonnetlebrun @marcosci!

maelle commented 3 years ago

@ropensci-review-bot approve

ropensci-review-bot commented 3 years ago

Approved! Thanks @vbaliga for submitting and @asbonnetlebrun, @marcosci 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). 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.

maelle commented 3 years ago

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

vbaliga commented 3 years ago

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 🐢

maelle commented 3 years ago

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

vbaliga commented 3 years ago

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

vbaliga commented 3 years ago

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

To-dos:

  • [x] 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.
  • [x] Fix all links to the GitHub repo to point to the repo under the ropensci organization.
 * [x]  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
 * [x]  If you already had a `pkgdown` website **and are ok relying only on [rOpenSci central docs building and branding](https://devguide.ropensci.org/ci.html#even-more-ci-ropensci-docs)**,
  * 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`
  • [x] 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)
  • [x] Please check you updated the package version post-review version updated and that you documented all changes in NEWS.md
  • [x] 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 3 years ago

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