Open chenzhao2020 opened 3 years ago
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing: 3
Hi Chirag, Steffen and Chen - great work on this package! I really like the abstraction and quick introduction to accessing weather observations from all over the world. I can definitely see myself using this for some personal projects with weather related data.
I found the package structure was spot on - testing coverage looks really thorough as well.
I haven't checked the Functionality box due to one small issue with using the time_basis="daily"
in plot_weather_data
. The issue can be reproduced with nearly the example given in the vignette:
library(noaastnr)
weather_data <- get_weather_data("911650-22536", 2020)
plot_weather_data(weather_data, col_name="air_temp", time_basis = "daily")
Which produces:
Error in as.Date.default(date) :
do not know how to convert 'date' to class “Date”
I believe the solution should just be changing month
to date
on this line
I think the tests are missing this because they aren't rendering the objects which would cause ggplot2
to evaluate the objects mapped in the graph. You could look at using vdiffr
to do visual regression tests to catch this plus other errors that may come up - I find it not too painful to use and it locks in desired visuals pretty well. Send me a message if you want more info.
Otherwise I noticed a few things that could potentially be tweaked to speed things up / reduce duplication if you wanted.
get_weather_data
: Instead of looping through the lines in the data that is returned, you can cast to tibble
and then use a dplyr::mutate
statement with vectorized stringr
statements to speed this up. From quick testing I think time for one call goes from ~ 2.5 minutes -> 5 seconds for the example weather station (I'll open a PR for this addition to show what I mean).
plot_weather_data
: Instead of duplicating all the plotting code except for the x-axis (either monthly or daily) and y-axis (temp, pressure etc), you could have one plotting function that takes both as arguments. This should reduce copy/paste mistakes when adding more options - I find I always make a small mistake!
There are just a few typos in your README - The "Dependancies" section header, and "publically" in your opening paragraph. In your vignette, you also reference the plot_weather_data
function returning an Altair object.
Thanks for making this package! I think the open source weather/geospatial space is really exciting and this package is a welcome addition to quickly grab data from nearby stations. The links to NOAA's weather station GIS portal is a welcome addition as well and helped me poke around what is available. Let me know if you have any questions or thoughts -
Dustin
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 am in the same MDS cohort as the authors of
noaastnr
package.
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
- Please refer to the comments.
Estimated hours spent reviewing: 3
Hi Noaastnr team,
Congratulation! You have come up with a great idea and very well structured package. I'm sure it would useful for so many people outside of the MDS. Your code is very easy to follow and readable, Great Job. I have couple of suggestions for you that you may consider:
get_weather_data()
function is quite slow. It would be good to vectorize your code to speed it up.plot_weather_data()
function. I went through your code and it seems you have not defined date
column. You can fix it by changing the line 254
in your noaastnr.R to: df <-
dplyr::group_by(df, date = lubridate::floor_date(datetime, "day"))
It would be good to mention your names in the License files as well instead of your group number.
It would be nice to briefly describe the inputs and outputs of your functions in your Roxygens.
In the following part of your documentation, I believe the Altair should be ggplot object.
The function returns an Altair chart object which can be saved or displayed in any environment which can render Altair objects.
plot_weather_data()
can be slightly drier by making a sepeare function for plotting. I believe it makes your code even more easier to follow.Great Job! I really enjoyed going through your package and I have learned something new as well. All the best, Kamal
Hello @dbandrews and @kmoravej , thank you so much for your comments, those are really helpful. Our team has discussed all the issues you mentioned, and we are going to address them as best as we can. Will keep you updated about our progress. Thanks again for your valuable feedbacks and help!
Thank you @dbandrews and @kmoravej for the helpful reviews! We have addressed the changes based on your suggestions in the latest pull requests. The details are as follows:
Dustin:
time_basis="daily"
in `plot_weather_data" functionplot_weather_data
functionplot_weather_data
function as ggplot objectKamal:
time_basis="daily"
in `plot_weather_data" functionplot_weather_data
function as ggplot objectplot_weather_data
functionOther fixes:
get_weather_data()
function to allow station numbers that begin with a letterPlease feel free to check them out. Let us know if you have any more suggestions or advices. Thank you very much!
Submitting Author: Chen Zhao (@chenzhao2020) Other Authors: Chirag Rank (@chiragrank), Steffen Pentelow (@spentelow) Repository: noaastnr Version submitted: 0.3.1 Editor: Tiffany Timbers Reviewers: TBD
Archive: TBD Version accepted: TBD
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.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
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 submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
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