Closed jsta closed 4 years ago
@TrashBirdEcology and @jsta Great to deal with issues in separate issues from the overall review like this. Also might make sense to have multiple separate issues for some of these items, depending on how separate and major they are. I'll let you organize though.
@jsta Thank you for the valuable feedback! Will address as soon as possible. Keeping open until all issues resolved or re-assigned.
In the spirit of providing an independent review I held off on looking at this issue until I was done. Now that I'm finished and taking a look it looks like @jsta & I highlighted a number of similar things in the UI, so :raised_hands: for consistent reviews!
Thanks, Ethan!
Jessica L. Burnett
GitHub http://github.com/trashbirdecology CV/Resume https://github.com/TrashBirdEcology/cv/blob/master/burnett_extendedCV.pdf ORC ID: 0000-0002-0896-5099 https://orcid.org/0000-0002-0896-5099
On Tue, Nov 5, 2019 at 12:43 PM Ethan White notifications@github.com wrote:
In the spirit of providing an independent review I held off on looking at this issue until I was done. Now that I'm finished and taking a look it looks like @jsta https://github.com/jsta & I highlighted a number of similar things in the UI, so 🙌 for consistent reviews!
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/TrashBirdEcology/bbsAssistant/issues/63?email_source=notifications&email_token=ACL2TNL7JQ6IG34KRB5YEG3QSGWC7A5CNFSM4JBPCMF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDDUZ7Y#issuecomment-549932287, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACL2TNJNOLR5T5MMG2IC6P3QSGWC7ANCNFSM4JBPCMFQ .
@jsta, in a major revamp of the functionality and the vignette, I have addressed each checked issue you opened above in branch joss-round1. Thanks for the feedback! Will address this in the JOSS review officially when we are ready to resubmit.
The nice-to-have region names has not yet been resolved. Hope to do this soon, though.
I figured I would start an issue thread here for the JOSS Review https://github.com/openjournals/joss-reviews/issues/1768
My first set of comments (1) are more or less "must-haves" to meet the JOSS requirements while the second set (2) are "nice-to-have" suggestions.
1_JOSS
[x]
paper.md
should probably have an explicit citation for therBBS
package.[x] The
testthat
tests intest-get_regions.R
fails for me with error:2_Nice-to-haves
User friendliness
The stated goal of the package is to make working with bbs data more user friendly by consolidating "the efforts of the data user by automating downloading and decompression of .zip data files, downloading route-level information, and saving them as .feather files for speedy import from disk."
As a new user, there were some areas that I thought could be even more user friendly:
[x] The biggest was the manual download > export > import process. Current operation is download to temporary file then manually export to "permanent" disk then later import this data. What if this was revised so that a user could optionally pass a permenent location to
get_bbsData
, this location would checked for an existing file (to avoid re-downloading existing files) and automatically exported?[ ] I wish the docs included some maps. For instance, as a new user I didn't know the difference between
Region=="S05"
andRegion.Code=="FLA"
[x] I felt like some of the calls to the
stringr
package in the vignette were not needed. Maybe people who don't knowstringr
would have an easier time if these were replaced with base R code? Maybe theregion_code
object could be cleaned up so that someone could runregion_codes[region_codes$State == "Florida", "zip_states"]
?[x] The vignette/package uses the feather package AND variables called
feather
. This could be confusing.[ ] Maybe the vignette could use a more descriptive title rather than "vignettes"?
Misc
[x] I had trouble understanding the
subset_speciesList
function. Why does it remove the specified family rather than include it likedplyr::filter
? What does this function offer beyonddplyr::filter
?[x] Why are the first and second figures in the vignette different? Seems like they are both plotting
AOU.Number=='s06882'
andRegion.Code=="S05"
by year.pkgdown
[x] Seems like the
pkgdown
site is out of date. The current landing page doesn't match the README.[x] Vignette images on the pkgdown site are broken for me...
[x] The vignette chunk
Subsetting the BBS count data by AOU number
prints A LOT of output. Maybe it would be good to pass the output throughglimpse
like the other chunk outputs?