search

openjournals / joss-reviews

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

[REVIEW]: Shiny tools for management rules: interactive applications that aid in conservation strategies for North Atlantic right whales #5436

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@leahcrowe<!--end-author-handle-- (Leah Crowe) Repository: https://github.com/NEFSC/READ-PSB-LWT-narwss_rwsas_apps Branch with paper.md (empty if default branch): Version: v1.1.0 Editor: !--editor-->@graciellehigino<!--end-editor-- Reviewers: @TanyaS08, @salix-d Archive: 10.5281/zenodo.8127594

Status

status

Status badge code:

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

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@TanyaS08 & @salix-d, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @graciellehigino know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @TanyaS08

📝 Checklist for @salix-d

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.04 s (1401.2 files/s, 206604.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               27            724            478           4914
XML                             12              0              1            757
HTML                             1             48              0            634
TeX                              3             44              7            293
Markdown                         2             43              0            112
Standard ML                      2              0              0            107
Rmd                              9             54            252             48
YAML                             2              3              4             29
-------------------------------------------------------------------------------
SUM:                            58            916            742           6894
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 1444

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

OK DOIs

- 10.1016/j.marpol.2019.02.019 is OK
- 10.3354/dao03376 is OK
- 10.1111/2041-210x.13244 is OK
- 10.3354/esr00368 is OK
- 10.5670/oceanog.2021.308 is OK
- 10.21105/joss.03094 is OK
- 10.3389/fmars.2021.760840 is OK

MISSING DOIs

- 10.1575/1912/29599 may be a valid DOI for title: North Atlantic Right Whale Consortium 2021 Annual Report Card

INVALID DOIs

- None
editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

graciellehigino commented 1 year ago

Hi all! Welcome to the review thread! 🎉

While our excellent reviewers gather their comments, could you please double-check the missing DOIs on the paper, @leahcrowe? Thanks!

@TanyaS08 and @salix-d - thank you so much for accepting to be part of this discussion! As the bot has mentioned, you can learn more about the review process here. Do not hesitate to contact me if you need anything. [=

TanyaS08 commented 1 year ago

Review checklist for @TanyaS08

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

leahcrowe commented 1 year ago

@editorialbot generate pdf

DOIs updated!

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

salix-d commented 1 year ago

Review checklist for @salix-d

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

salix-d commented 1 year ago

Hi!

I was trying to test the functionnality with the example_data/20210409. I found the documentation wiki to be quite clear, and was easily able to follow along. However, I was missing some information to be able to test fully.

I checked 'local' and entered the path where I saved the files in that folder, entered '210409' as the 'Survey Date', but I couldn't find the 'Tail number'. I tried without and got the error:

Uh oh! Those files can't be found! Double check your connection to the network, the local network pathway, your data, and/or your survey date entry. whether 'Edited eff/sig files' had 'yes' or 'no' selected.

Do you have the tail number somewhere in this repo? If not, I would add it in a text file in the 'example_data/20210409' folder.

Another thing is that you need an 'Example usage'. I think you could easily add with the example data so people can get a feel of what can be done.

Thanks!

TanyaS08 commented 1 year ago

I'm going to attach/insert myself here as well since I had the same issue (I also added some dummy digits for a flight number - same message). I was wondering if it might also be an OS issue when setting the path? I'm running macOS for what it's worth...

salix-d commented 1 year ago

And I'm on windows.

( kinda nice we cover multiple OS to test :) )

leahcrowe commented 1 year ago

Hi all,

Thank you for agreeing to test this app. We are grateful for your involvement and review!

You don't need to include a tail number to proceed, but I have updated the wiki page with an example tail number: https://github.com/NEFSC/READ-PSB-LWT-narwss_rwsas_apps/wiki/Aerial-Survey-Processing-App,-Aerial-Survey-Tab:-Part-1

The issue you all were running into was that I incorrectly named the folder containing the example data (was YYYYMMDD when it should have been YYMMDD). All good now, and I have updated the README file with how to run the example data (without needing to download anything to your machine).

I'm aware that you all will not be able to test the trigger evaluation without access to our database server, so I am working on an example workaround.

Hopefully at least the processing app goes a bit more smoothly for you -- sorry about that mix up!

Leah

salix-d commented 1 year ago

The problem was that I thought the path to the files included the folder 210409 and/or that they needed to be in a folder named with the date.

Perhaps " the pathway you enter should be the pathway where the edit files are stored." could be change to something like " the pathway you enter should be the pathway to the folder containing the edit files are stored. This file should also be labeled like [YYMMDD]."

salix-d commented 1 year ago

For the dependencies, I notice that if I don't load the libraries, they get loaded when doing shiny::runGitHub("READ-PSB-LWT-narwss_rwsas_apps", username = "NEFSC", ref = "master") so the user doesn't have to copy the list from the readme.

Since the review check list mentionned that

Ideally these should be handled with an automated package management solution.

I would suggest using require instead of library in the global_libraries.R script. This will install any missing packages for the user.

salix-d commented 1 year ago

I was able to download the report in html but not in pdf format.

I had this error :

An error has occurred! LaTeX failed to compile C:\Users\salix\AppData\Local\Temp\RtmpwVL36t\file39543fa5181d.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file39543fa5181d.log for more info.

Here's the log file: file39543fa5181d.log

Don't know if related, but the html file was named "09Apr2021_NOAA_NERW_Aerial_Report.html" and the pdf file "report.htm".

leahcrowe commented 1 year ago

I was able to download the report in html but not in pdf format.

I had this error :

An error has occurred! LaTeX failed to compile C:\Users\salix\AppData\Local\Temp\RtmpwVL36t\file39543fa5181d.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file39543fa5181d.log for more info.

Here's the log file: file39543fa5181d.log

Don't know if related, but the html file was named "09Apr2021_NOAA_NERW_Aerial_Report.html" and the pdf file "report.htm".

This is an issue that occurs related to tinytex/your machine's LaTeX distribution. I offer up some tests/installation instructions in the "Additional installation" README section. I have been able to set this up on several different computers and our shiny server, but some computers have eluded me, which is why we have offered the html download option. The correct naming convention is added to the file when it works properly.

leahcrowe commented 1 year ago

Hi all,

I've added in three more example days of data that offer different scenarios for the trigger analysis from the aerial survey data (see the README file for the update). For our functional operations, we have this app interacting on our internal server and database, so I can't offer up that actual experience, but I hope the examples I have provided demonstrate how this app is going through the decision making process for triggering protection zones, or not. Please let me know if there is anything further you would like to see to proceed.

Leah

TanyaS08 commented 1 year ago

I was able to download the report in html but not in pdf format. I had this error :

An error has occurred! LaTeX failed to compile C:\Users\salix\AppData\Local\Temp\RtmpwVL36t\file39543fa5181d.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file39543fa5181d.log for more info.

Here's the log file: file39543fa5181d.log Don't know if related, but the html file was named "09Apr2021_NOAA_NERW_Aerial_Report.html" and the pdf file "report.htm".

This is an issue that occurs related to tinytex/your machine's LaTeX distribution. I offer up some tests/installation instructions in the "Additional installation" README section. I have been able to set this up on several different computers and our shiny server, but some computers have eluded me, which is why we have offered the html download option. The correct naming convention is added to the file when it works properly.

I've also had an issue with creating the PDF (but everything else works! 😊). My error message looks a bit different though and says the following: I was unable to find any missing LaTeX packages from the error log file947ed0e2974.log. ! Undefined control sequence. l.81 title: ``NOAA Northeast Region \nRight Whale Aerial Survey Report''

which is then followed by:

Warning: Error in : LaTeX failed to compile /var/folders/wf/wmp7x6xj7_zbx2bl9z5ysfwm0000gn/T//Rtmpuirwmm/file947ed0e2974.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file947ed0e2974.log for more info.

It could be that I also have an underlying LATEX issue but I was wondering about the l.81 title: ``NOAA Northeast Region \nRight Whale Aerial Survey Report'' comment...

leahcrowe commented 1 year ago

I was able to download the report in html but not in pdf format. I had this error :

An error has occurred! LaTeX failed to compile C:\Users\salix\AppData\Local\Temp\RtmpwVL36t\file39543fa5181d.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file39543fa5181d.log for more info.

Here's the log file: file39543fa5181d.log Don't know if related, but the html file was named "09Apr2021_NOAA_NERW_Aerial_Report.html" and the pdf file "report.htm".

This is an issue that occurs related to tinytex/your machine's LaTeX distribution. I offer up some tests/installation instructions in the "Additional installation" README section. I have been able to set this up on several different computers and our shiny server, but some computers have eluded me, which is why we have offered the html download option. The correct naming convention is added to the file when it works properly.

I've also had an issue with creating the PDF (but everything else works! 😊). My error message looks a bit different though and says the following: I was unable to find any missing LaTeX packages from the error log file947ed0e2974.log. ! Undefined control sequence. l.81 title: ``NOAA Northeast Region \nRight Whale Aerial Survey Report''

which is then followed by:

Warning: Error in : LaTeX failed to compile /var/folders/wf/wmp7x6xj7_zbx2bl9z5ysfwm0000gn/T//Rtmpuirwmm/file947ed0e2974.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file947ed0e2974.log for more info.

It could be that I also have an underlying LATEX issue but I was wondering about the l.81 title:NOAA Northeast Region \nRight Whale Aerial Survey Report'' `` comment...

I am thinking this has to do with your LaTeX distribution and if it recognizes "\n" as a line break or if it is looking for a different command. Likely it doesn't like the single "\" in how your machine is set up. Our team just compiled the attached report the other day, so we do have this up and running properly on our server, but it's also exactly why we also offer the html option!

09May2023_NOAA_NERW_Aerial_Report.pdf

TanyaS08 commented 1 year ago

I was able to download the report in html but not in pdf format. I had this error :

An error has occurred! LaTeX failed to compile C:\Users\salix\AppData\Local\Temp\RtmpwVL36t\file39543fa5181d.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file39543fa5181d.log for more info.

Here's the log file: file39543fa5181d.log Don't know if related, but the html file was named "09Apr2021_NOAA_NERW_Aerial_Report.html" and the pdf file "report.htm".

This is an issue that occurs related to tinytex/your machine's LaTeX distribution. I offer up some tests/installation instructions in the "Additional installation" README section. I have been able to set this up on several different computers and our shiny server, but some computers have eluded me, which is why we have offered the html download option. The correct naming convention is added to the file when it works properly.

I've also had an issue with creating the PDF (but everything else works! 😊). My error message looks a bit different though and says the following: I was unable to find any missing LaTeX packages from the error log file947ed0e2974.log. ! Undefined control sequence. l.81 title: NOAA Northeast Region \nRight Whale Aerial Survey Report'' which is then followed by: Warning: Error in : LaTeX failed to compile /var/folders/wf/wmp7x6xj7_zbx2bl9z5ysfwm0000gn/T//Rtmpuirwmm/file947ed0e2974.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See file947ed0e2974.log for more info. It could be that I also have an underlying LATEX issue but I was wondering about the `l.81 title:`NOAA Northeast Region \nRight Whale Aerial Survey Report'' comment...

I am thinking this has to do with your LaTeX distribution and if it recognizes "\n" as a line break or if it is looking for a different command. Likely it doesn't like the single "" in how your machine is set up. Our team just compiled the attached report the other day, so we do have this up and running properly on our server, but it's also exactly why we also offer the html option!

09May2023_NOAA_NERW_Aerial_Report.pdf

I'm wondering if there is a way to word this issue better in the README/wiki. I looked at it again and it is there but maybe in the README under the 'Additional installations' section it might be nice to have a sub-sub heading that is something along the lines of 'troubleshooting for PDF report generation'? And a phrase that echos what is in the README that PDF generation is hard but try these links for troubleshooting or generate the report as an .html and convert to PDF from there. Having just written that it might clog up the README so maybe even just a simple hyperlink linking to the wiki (https://github.com/NEFSC/READ-PSB-LWT-narwss_rwsas_apps/wiki/Aerial-Survey-Processing-App,-Aerial-Survey-Tab:-Part-3) to make the link more clear/make the user aware of this potential issue?

TanyaS08 commented 1 year ago

Hopefully I've done this right - I've opened an issue regarding documentation here:

https://github.com/NEFSC/READ-PSB-LWT-narwss_rwsas_apps/issues/26

salix-d commented 1 year ago

Hello,

Regarding the functionality check: The parts of the app that we have access to do seem to work properly, but a lot of it can't be tested (like all the triggers).

I'm unsure if this is an issue for the paper. If it is, I did find this package https://rstudio.github.io/shinytest/articles/shinytest.html. I'm assuming you did do manual testing before submitting, but, maybe having a script showing those tests on your setup might help? I think it would certainly help in the long run regardless.

And if it's not, then for me, everything on my checklist is checked 😄

graciellehigino commented 1 year ago

Hi folks! This review is going great and it's close to the conclusion. I see great improvements in the documentation, congrats! 🎉

@leahcrowe I think @salix-d 's comment about testing is pertinent and it would be great if you could implement that. It shouldn't refrain us from moving forward to acceptance, though, as long as there are descriptions on how users can test the functionality of the app manually.

@TanyaS08 I see there are a couple of boxes on your checklist still to be checked. Do you have further comments about the paper?

TanyaS08 commented 1 year ago

I've had a look at everything and I'm happy to have this software + manuscript moved forward! ✈️ I've also closed the one issue I opened relating to the ms

leahcrowe commented 1 year ago

Thank you for the testing suggestion. I had trouble getting the shinytest to work (I think because of all the html), but I have gotten shinytest2 (https://rstudio.github.io/shinytest2/) to work locally, so I think that will be what we use to demonstrate the functionality specific to being connected to the network server/accessing internal databases.

There are some travel conflicts this week, but our hope is to work this out next week!

Thanks to everyone for you helpful suggestions to improve the project!

leahcrowe commented 1 year ago

Hello all!

I have added in the dates for manual testing of the Trigger Analysis app in the wiki here. For instances where a protection zone is triggered and/or extended, I have also uploaded the reports to those locations.

Unfortunately, I keep running into snags with the shinytest packages. I think it boils down to their requirement of only 1 Rmd file allowed per app. We have several Rmd files in this project, so without substantial workarounds, I don't think I can get this to work (I could get it to work with other projects I have that have only one Rmd).

The main difference between declaring a protection zone through the "Trigger Analysis" route and from sightings data analyzed in the "Aerial Survey Processing App" is the source of the data: "Trigger Analysis" queries data by date from all sources in our Oracle database (both visual and acoustic, opportunistic and dedicated), and the "Aerial Survey Processing App" only looks at sightings from a visual aerial survey. The process that analyzes the detections to see if they meet the criteria for a dynamic protection zone are the same, and occur through the shared files.

Other than record a video of the process, I am not sure what else we can do without substantial code changes. But let me know if you all have other ideas!

salix-d commented 1 year ago

Hello!

I'm not really familiar with shiny app testing (I found the package with a small web search), so I'm not sure what to suggest.

I also understand why some features are limited to people with access to the database, and the part we have access to run properly. I think with the documentation added for manual testing, it should be good 🙂

salix-d commented 1 year ago

@graciellehigino What's the next step?

graciellehigino commented 1 year ago

Thank you all for putting so much work into this publication! It sounds great to me - and the good thing about this review being open is that everyone can read what you just explained, @leahcrowe [=

So I guess we can start the final checks and recommend publication! Congrats! 🎉

@salix-d and @TanyaS08 - thank you SO MUCH for your contribution! You don't need to worry about anything else from now on. I'll take care of the final reviews. If you enjoyed the experience and want to be listed on the JOSS reviewers database, feel free to register here: https://reviewers.joss.theoj.org/ (you just need to authorize the app with your github account)

graciellehigino commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

graciellehigino commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.3354/esr01137 is OK
- 10.1016/j.marpol.2019.02.019 is OK
- 10.1575/1912/66099 is OK
- 10.3354/dao03376 is OK
- 10.1111/2041-210x.13244 is OK
- 10.3354/esr00368 is OK
- 10.1098/rsos.180892 is OK
- 10.1038/s41598-017-13359-3 is OK
- 10.5670/oceanog.2021.308 is OK
- 10.21105/joss.03094 is OK
- 10.3389/fmars.2021.760840 is OK

MISSING DOIs

- None

INVALID DOIs

- None
graciellehigino commented 1 year ago

@leahcrowe can you help do the final checks? =D

leahcrowe commented 1 year ago

@leahcrowe can you help do the final checks? =D

Yes! Happy to help! I am currently in the field, but will get to this as soon as I can. Thanks, all!

leahcrowe commented 1 year ago

Hello all,

Sorry for the delay -- I was in the field with limited internet. See below! Let me know if there are any other items I should address.

Leah

Additional Author Tasks After Review is Complete

graciellehigino commented 1 year ago

@editorialbot set https://doi.org/10.5281/zenodo.8127594 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8127594

graciellehigino commented 1 year ago

@editorialbot set v1.1.0 as version

editorialbot commented 1 year ago

Done! version is now v1.1.0

graciellehigino commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

graciellehigino commented 1 year ago

Hi @leahcrowe ! Thank you so much for doing these checks with me. I've skimmed through the manuscript and I only noticed one thing that may be an error: one of the authors in this reference below has only their initials. Can you double-check that too? We're almost there! 🎉

Meyer-Gutbrod, E. L., Greene, C. H., Davies, K. T., & G, J. D. (2021). Ocean regime shift is driving collapse of the north atlantic right whale population. Oceanography. https://doi.org/10.5670/oceanog.2021.308

graciellehigino commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.3354/esr01137 is OK
- 10.1016/j.marpol.2019.02.019 is OK
- 10.1575/1912/66099 is OK
- 10.3354/dao03376 is OK
- 10.1111/2041-210x.13244 is OK
- 10.3354/esr00368 is OK
- 10.1098/rsos.180892 is OK
- 10.1038/s41598-017-13359-3 is OK
- 10.5670/oceanog.2021.308 is OK
- 10.21105/joss.03094 is OK
- 10.3389/fmars.2021.760840 is OK

MISSING DOIs

- None

INVALID DOIs

- None
leahcrowe-otago commented 1 year ago

Hi @leahcrowe ! Thank you so much for doing these checks with me. I've skimmed through the manuscript and I only noticed one thing that may be an error: one of the authors in this reference below has only their initials. Can you double-check that too? We're almost there! 🎉

Meyer-Gutbrod, E. L., Greene, C. H., Davies, K. T., & G, J. D. (2021). Ocean regime shift is driving collapse of the north atlantic right whale population. Oceanography. https://doi.org/10.5670/oceanog.2021.308

Nice catch! I just pushed the edit!

graciellehigino commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

graciellehigino commented 1 year ago

@editorialbot recommend-accept