search

adithirgis / pollucheck

This application helps exploring the open source air quality data.
Other
10 stars 6 forks source link

JOSS review #2

Closed nmstreethran closed 3 years ago

nmstreethran commented 3 years ago

Hi @adithirgis. I'm opening this issue as part of my review for JOSS (openjournals/joss-reviews#3435). You can find my initial comments below. I'll go through your submission again once you make some changes and let you know if I have any further comments. Please feel free to ping me if anything is unclear.

Thanks!

General comments

You seem to have renamed this repository from OpenSourceAirQualityApp to pollucheck. However, in a number of files, the repository's name and URL still point to the old name. These need to be fixed. The Shiny app is still called OpenSourceAirQualityApp as well, but this is not an issue, so it's up to you if you want to rename it.

Some wording should be edited for consistency. Use either:

Minor corrections:

README

In line 9, state what CPCB stands for. In line 11, I think it will be useful to add a hyperlink to the openair package.

The section headings are quite long. I think you could use shorter headings, followed by a description below. If you try to use GitHub's table of contents to navigate through the README, it will be difficult to make out the heading titles.

Some of the images are not indented correctly. Can you indent them so that they align with their corresponding bullet point?

Could you also add the installation instructions to serve the app locally here?

Community guidelines

I think no description is needed for point 1; just add a link to CONTRIBUTING.md. I suggest adding a disclaimer that contributors must adhere to the code of conduct, and a link to it.

Author credit statement

This is a very long sentence; do you think you can split it up?

Contributing

The link to the Code of Conduct in CONTRIBUTING.md is broken. You can move the Code of Conduct to .github to fix this.

Fixing typos

It would be good to include PR instructions in this section. You have suggested using the GitHub interface directly, so you can add a link to the relevant GitHub documentation for this (e.g., "Creating a pull request from a fork").

"... as long as the changes are made in the source file", as well as the roxygen2 description, might be a bit confusing to new contributors. If a contributor wants to fix a typo in the README, then it won't be necessary for them to learn about roxygen2 comments. My suggestion is to clarify that if contributors find any typos in an .Rd file, then they should make changes in the corresponding .R file, as .Rd files are automatically generated by roxygen2 and should not be edited by hand.

Bigger changes

I think install.packages("devtools") should be the first step here. My base R environment did not include devtools, so I had to install it first.

You can probably remove "If you haven't done this before", since this is targeted towards those already familiar with Git and R/tidyverse. I think a link to usethis's documentation would be useful here though.

In point 2, you mention that devtools::check() should pass cleanly. However, I got a note regarding the package size when I ran the check:

── R CMD check results ─────────────────────────────────── pollucheck 1.0.0 ────
Duration: 12.8s

❯ checking installed package size ... NOTE
    installed size is  7.6Mb
    sub-directories of 1Mb or more:
      shiny   7.5Mb

0 errors ✔ | 0 warnings ✔ | 1 note ✖

I don't know how this note can be fixed. Perhaps you could clarify that the check should pass without errors or warnings.

Point 4: "git" -> "Git".

Point 5: "For user-facing changes, add a bullet to the top of NEWS.md (i.e. just below the first header)." There is no NEWS.md at present, so you could create one and add some placeholder text.

DESCRIPTION

Suggested changes by line number:

App

These things were picked up by the linter in inst/shiny/app.R:

Paper

Summary

In the second sentence of paragraph 1, there is a comma missing between "actions" and "quantification".

In the second paragraph, I think you need to introduce pollucheck first instead of shiny. Something along the lines of "pollucheck is an interactive R application built using the shiny package...", where you add a citation to the shiny package.

I think it's important to mention in the summary that this app is for exploring Indian air pollution data.

You mention in the "App Display" section the different packages used for building pollucheck. I think you can move that sentence to the summary, so that this section focusses on the app's functionality.

Limitations

In point 5, can you briefly elaborate why caution must be exercised regarding wind direction data? You stated in DESCRIPTION that only hourly data is processed correctly. Can you state that here?

Installation

The heading level for the "Installation" seems incorrect.

I think it's useful to state here that the app is already hosted online on shinyapps.io, but users can choose to serve it locally using the installation instructions provided.

I recommend installing devtools as the first step. Additionally, if you are importing the pollucheck library, it won't be necessary to use the pollucheck:: prefix to run the app. Therefore, I think your installation commands could be changed to one of the following:

install.packages("devtools")
devtools::install_github("adithirgis/pollucheck")
library(pollucheck)
pollucheck_run()

# or...

install.packages("devtools")
devtools::install_github("adithirgis/pollucheck")
pollucheck::pollucheck_run()

In the last paragraph:

Case Study

It will be good to include some information about the data used for the case study. You say it's based on 18 months of pollution data -- what's the study area and what are the data sources?

In paragraph 3, "Interquartile" should be "interquartile".

Bibliography

The bibliography entries' titles are not capitalised correctly in the compiled PDF. To preserve capitalisation, you can wrap the titles in quotes. For example:

title = "{Air pollution in mega cities in China}"

There is also some spacing after the decimal/dot in some of the titles:

adithirgis commented 3 years ago

Thanks for a thorough review, we will get back to you soon with the changes.

adithirgis commented 3 years ago

Hello @nmstreethran,

Thanks for a thorough review, I have updated all the comments you have provided. Though there is a small clarification regarding the data used here -

Please let us know if we need to change anything.

Cheers! Adithi

nmstreethran commented 3 years ago

Hi @adithirgis, thank you for the quick update! I'll go through your submission again and let you know by the end of today if there is anything else to address.

Thanks for the clarification regarding the supported data sources. Your README still says that "pollucheck helps exploring the open-source Indian air quality data", and the OpenAQ and AirNow links point to data specific to India. Could you update this?

These links seem more appropriate:

adithirgis commented 3 years ago

Thanks so much for these as well. We have updated now the documentation as well.

Cheers! Adithi

nmstreethran commented 3 years ago

Thanks @adithirgis. I have just a few minor comments left:

DESCRIPTION

paper.md

inst/shiny/WWW/include.Rmd

inst/shiny/app.R

README.md

nmstreethran commented 3 years ago

@adithirgis Additionally, there were a few conflicts and a note regarding a deprecated function when I served the app locally using pollucheck::pollucheck_run(). You can find the log here:

Loading required package: shiny
── Attaching packages ───────────────────────────────────────────────────── tidyverse 1.3.1 ──
✔ ggplot2 3.3.5     ✔ purrr   0.3.4
✔ tibble  3.1.2     ✔ dplyr   1.0.7
✔ tidyr   1.1.3     ✔ stringr 1.4.0
✔ readr   2.0.0     ✔ forcats 0.5.1
── Conflicts ──────────────────────────────────────────────────────── tidyverse_conflicts() ──
✖ dplyr::filter() masks stats::filter()
✖ dplyr::lag()    masks stats::lag()
Stackoverflow is a great place to get help:
        https://stackoverflow.com/tags/shinyjs

Attaching package: ‘shinyjs’

The following object is masked from ‘package:shiny’:

    runExample

The following objects are masked from ‘package:methods’:

    removeClass, show

Registered S3 method overwritten by 'quantmod':
  method            from
  as.zoo.data.frame zoo 
biwavelet 0.20.21 loaded.

Attaching package: ‘biwavelet’

The following object is masked from ‘package:ggplot2’:

    arrow

Attaching package: ‘DT’

The following objects are masked from ‘package:shiny’:

    dataTableOutput, renderDataTable

Attaching package: ‘broom’

The following object is masked from ‘package:bslib’:

    bootstrap

data.table 1.14.0 using 2 threads (see ?getDTthreads).  Latest news: r-datatable.com

Attaching package: ‘data.table’

The following objects are masked from ‘package:dplyr’:

    between, first, last

The following object is masked from ‘package:purrr’:

    transpose

Attaching package: ‘janitor’

The following objects are masked from ‘package:stats’:

    chisq.test, fisher.test

Attaching package: ‘zoo’

The following objects are masked from ‘package:base’:

    as.Date, as.Date.numeric

Listening on http://127.0.0.1:3542
New names:                                                                    
* `` -> ...3
* `` -> ...4
* `` -> ...5
* `` -> ...6
Warning in FUN(X[[i]], ...) : NAs introduced by coercion
Warning in FUN(X[[i]], ...) : NAs introduced by coercion
Warning in FUN(X[[i]], ...) : NAs introduced by coercion
Warning in FUN(X[[i]], ...) : NAs introduced by coercion
`mutate_all()` ignored the following grouping variables:
Columns `day`, `Location`
Use `mutate_at(df, vars(-group_cols()), myoperation)` to silence the message.
Warning: `funs()` was deprecated in dplyr 0.8.0.
Please use a list of either functions or lambdas: 

  # Simple named list: 
  list(mean = mean, median = median)

  # Auto named with `tibble::lst()`: 
  tibble::lst(mean, median)

  # Using lambdas
  list(~ mean(., trim = .2), ~ median(., na.rm = TRUE))
This warning is displayed once every 8 hours.
Call `lifecycle::last_warnings()` to see where this warning was generated.
nmstreethran commented 3 years ago

This is the output of lifecycle::last_warnings():

[[1]]
<deprecated>
message: `funs()` was deprecated in dplyr 0.8.0.
Please use a list of either functions or lambdas: 

  # Simple named list: 
  list(mean = mean, median = median)

  # Auto named with `tibble::lst()`: 
  tibble::lst(mean, median)

  # Using lambdas
  list(~ mean(., trim = .2), ~ median(., na.rm = TRUE))
Backtrace:
  1. pollucheck::pollucheck_run()
 72. dplyr::funs(mean, sd)
adithirgis commented 3 years ago

Hello @nmstreethran!

Thank so much for pointing the deprecated function.

README.md line 109: "outliers" is spelled incorrectly.

I am not able to get this wrong spelling could you please point it out? Is it spelled outliers?

Thanks again Cheers! Adithi

adithirgis commented 3 years ago

Hello @nmstreethran!

We have implemented all your suggestions.

Thanks! Adithi

nmstreethran commented 3 years ago

Hi @adithirgis, thank you very much!

Here is the spelling mistake in README (line 110):

Specify a multiple (X) to remove outliers based on Mean and SD- If you want to clean your data set based on outliers, not usually necessary, use only if you want to remove outiliers based on Mean and Standard Deviation values.

You may close this issue once you correct this typo. I'll inform the editor that my review is complete.

Great work on the app!

adithirgis commented 3 years ago

Hello @nmstreethran,

Thanks for reviewing and helping us improve this application. I have fixed the typo.

Cheers! Adithi

Do we need to close this issue?

nmstreethran commented 3 years ago

@adithirgis, no problem! I'll close the issue now.