Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.0.3 #119

Merged
merged 22 commits into from
Aug 13, 2024
Merged

1.0.3 #119

merged 22 commits into from
Aug 13, 2024

Conversation

mitchellmanware
Copy link
Collaborator

@mitchellmanware mitchellmanware commented Aug 8, 2024

Updates from CRAN Submission amadeus 1.0.2

Italicized are the comments from the reviewer, and bullet points are my changes to address comment the comment or an explanation of why it was not adopted.


Please reduce the length of the title to less than 65 characters.

  • "Accessing and Analyzing Large Scale Environmental Data in R"
  • New title matches AGU abstract title.
  • 652903c

If there are references describing the methods in your package, please
add these in the description field of your DESCRIPTION file in the form
authors (year) doi:...
authors (year, ISBN:...)
or if those are not available: <https:...>
with no space after 'doi:', 'https:' and angle brackets for
auto-linking. (If you want to add a title as well please put it in
quotes: "Title”)


Please always add all authors, contributors and copyright holders in the
Authors@R field with the appropriate roles.
 From CRAN policies you agreed to:
"The ownership of copyright and intellectual property rights of all
components of the package must be clear and unambiguous (including from
the authors specification in the DESCRIPTION file). Where code is copied
(or derived) from the work of others (including from R itself), care
must be taken that any copyright/license statements are preserved and
authorship is not misrepresented.
Preferably, an ‘Authors@R’ would be used with ‘ctb’ roles for the
authors of such code. Alternatively, the ‘Author’ field should list
these authors as contributors. Where copyrights are held by an entity
other than the package authors, this should preferably be indicated via
‘cph’ roles in the ‘Authors@R’ field, or using a ‘Copyright’ field (if
necessary referring to an inst/COPYRIGHTS file)."
e.g.: -> "Spatiotemporal Exposures and Toxicology Group" in your LICENSE
file
Please explain in the submission comments what you did about this issue.


  • “Spatiotemporal Exposures and Toxicology Group” has been added to the Authors@R field as a copyright holder.
  • This change reflects the COPYRIGHT HOLDER section of the LICENSE file.
  • cd105e9

Please add \value to .Rd files regarding exported methods and explain
the functions results in the documentation. Please write about the
structure of the output (class) and also what the output means. (If a
function does not return a value, please document that too, e.g.
\value{No return value, called for side effects} or similar)
Missing Rd-tags:
calc_check_time.Rd: \value
calc_message.Rd: \value
check_for_null_parameters.Rd: \value
check_mysf.Rd: \value
check_mysftime.Rd: \value
download_data.Rd: \value
download_permit.Rd: \value
download_remove_command.Rd: \value
download_remove_zips.Rd: \value
download_run.Rd: \value
download_sink.Rd: \value
download_unzip.Rd: \value
test_download_functions.Rd: \value

  • \value tags updated to describe output/processes even when function has no return into the R environment.
  • cd105e9

\dontrun{} should only be used if the example really cannot be executed
(e.g. because of missing additional software, missing API keys, ...) by
the user. That's why wrapping examples in \dontrun{} adds the comment
("# Not run:") as a warning for the user. Does not seem always
necessary. Please replace \dontrun with \donttest.
Please unwrap the examples if they are executable in < 5 sec, or replace
dontrun{} with \donttest{}.
Please put functions which download data in \donttest{}.

  • Download function examples have download = FALSE, so there is no data downloaded.
  • cd105e9
  • Examples for the process_*() and calc_*() functions remain in the /donttest brackets.
  • These examples require spatial data to run without errors, and including the data (by including tests/testdata in build [currently on .Rbuildignore] or moving data to inst/extdata) inflates the package size substantially.
  • A small spatial subset (~3 x 3 grid cells) are used for tests, but still make the package very large (>338 Mb)
  • Below is a log snapshot from a build check with example data moved to inst/extdata.
─  checking whether package ‘amadeus’ can be installed ... [95s/18s] OK (17.9s)
N  checking installed package size ...
     installed size is 338.4Mb
     sub-directories of 1Mb or more:
       extdata  336.7Mb
       help       1.3Mb
✔  checking package directory

You write information messages to the console that cannot be easily
suppressed.
It is more R like to generate objects that can be used to extract the
information a user is interested in, and then print() that object.
Instead of print()/cat() rather use message()/warning() or
if(verbose)cat(..) (or maybe stop()) if you really have to write text to
the console. (except for print, summary, interactive functions)
-> R/download.R

  • The series of download_*() functions cat() command line commands to a .txt file on the user’s machine in order to download data files from URLs.
  • cat() is only used for writing these commands to text file.
  • The message() and stop() commands are used to provide the user with progress and error messages, respectively.


Please ensure that your functions do not write by default or in your
examples/vignettes/tests in the user's home filespace (including the
package directory and getwd()). This is not allowed by CRAN policies.
Please omit any default path in writing functions. In your
examples/vignettes/tests you can write to tempdir().
-> download-functions and their examples

  • Download function examples have been updated to utilize tempdir().
  • cd105e9
  • The workflow.Rmd did not write to the specified files as the chunks were set to eval = FALSE, but they have also been updated to use the tempdir() writing location.
  • Tests have been updated to use tempdir() instead of manually creating/writing/removing directories within the “testdata” folder.
  • 652903c

Functionality Fixes

download_run

  • Beta testing by Umit Tokac revealed that the download_*() functions were not downloading any data files when run on Windows OS
  • Problem was related to file type and command executer that were default in download_run for all OS
  • New steps
    1. Detects operating system
    2. Sets runner as ". " (Mac/Linux) or "" (Windows)
    3. Renames file from "commands.txt" to "commands.bat" (Windows only)
    4. Writes system command
    5. Runs download
    6. Runs download_remove_command
  • New steps 4 and 6 conslidate 10+ lines of code from each download_*() function into the download_run command while retaining all functionality and tests
    download_run <- function(
Screenshot 2024-08-08 at 1 06 47 PM - Function successfully downloads data files on Windows OS

image

download_remove_zips

  • Hot fix to stop function from deleting directory with data and parent directory
  • Caught by @sigmafelix and could have lead to significant data deletion
  • Until this pull request is complete, use pak::pak("NIEHS/amadeus@cran-0801") to load amadeus in order to avoid accidental data deletion @kyle-messier @eva0marques @eva0marques
  • f3868d6

Other

  • Line breaks in @references section for cleaner references in pkgdown website
  • Unit tests for calc_lagged(geom = TRUE) (did not work in last pull request for unknown reason)

@mitchellmanware
Copy link
Collaborator Author

The unit test for the calc_lagged function with geom = TRUE exhibits very strange behavior. The testthat::expect_no_error unit tests run without error, but when narr_lag_geom_setcols is called in the specific testthat::expect_true tests, it cannot be found.

This error was also occurring on a previous PR (39f9b33 and 3d17f5e) without reason.

The tests run and pass locally, so I am not sure what is causing this error. I removed the specific tests for now.

Screenshot 2024-08-08 at 2 20 55 PM Screenshot 2024-08-08 at 2 19 39 PM

- Documentation typo in `download_modis` fixed: LPDAAC->LAADS
- Link added
@sigmafelix
Copy link
Collaborator

@mitchellmanware I think the updated codes are all good with my quick update #120. Thank you!

MOD06_L2 link retrieval documentation
Copy link
Collaborator

@kyle-messier kyle-messier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm the "R CMD check --as-cran" check and the windows utility

@mitchellmanware
Copy link
Collaborator Author

@kyle-messier @sigmafelix
All GitHub Actions (which run with --as-cran) pass with Status: OK (no notes, warnings, or errors).

Check on win-builder is complaining about "Serre" in the calc_sedc citation which has been included in the package description paragraph. It also produced a spell check note for "et al., ..." instead of the name. I do not know how to add the citation if both of these throw a NOTE. Check is also reporting a NOTE for the https://epsg.io URL. This has not caused NOTES on win-builder in the past and does not report on GitHub actions or local runs.

Local check passes with NOTES and a WARNING, but we can ignore these. The first NOTE just reports the maintainer name and "new submission". The second NOTE reports that the HTML manual cannot be checked because tidy is not installed on triton. The WARNING is related to the check not identifying the qpdf package (which is installed).

> rcmdcheck::rcmdcheck(args = c("--as-cran"))
── R CMD build ─────────────────────────────────────────────────────────────────────────────────────
✔  checking for file ‘.../DESCRIPTION’ ...
─  preparing ‘amadeus’: (3s)
✔  checking DESCRIPTION meta-information ...
─  installing the package to process help pages
   Loading required namespace: amadeus
─  saving partial Rd database (5.2s)
✔  creating vignettes (5.8s)
─  checking for LF line-endings in source and make files and shell scripts
─  checking for empty or unneeded directories
   Removed empty directory ‘amadeus/inst/extdata/data_files’
   Removed empty directory ‘amadeus/inst/extdata/zip_files’
─  building ‘amadeus_1.0.3.tar.gz’
   
── R CMD check ─────────────────────────────────────────────────────────────────────────────────────
─  using log directory ‘/tmp/RtmpJyLNev/file27500add0ed15/amadeus.Rcheck’
─  using R version 4.3.2 (2023-10-31)
─  using platform: x86_64-pc-linux-gnu (64-bit)
─  R was compiled by
       gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18)
       GNU Fortran (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18)
─  running under: Rocky Linux 8.8 (Green Obsidian)
─  using session charset: UTF-8
─  using option ‘--as-cran’
✔  checking for file ‘amadeus/DESCRIPTION’
─  this is package ‘amadeus’ version ‘1.0.3’
─  package encoding: UTF-8
─  checking CRAN incoming feasibility ... [5s/67s] NOTE (1m 7.3s)
   Maintainer: ‘Kyle Messier <kyle.messier@nih.gov>’
   
   New submission
✔  checking package namespace information ...
✔  checking package dependencies (609ms)
✔  checking if this is a source package
✔  checking if there is a namespace
✔  checking for executable files (497ms)
✔  checking for hidden files and directories ...
✔  checking for portable file names ...
✔  checking for sufficient/correct file permissions ...
─  checking whether package ‘amadeus’ can be installed ... [94s/17s] OK (17.3s)
✔  checking installed package size ...
> rcmdcheck::rcmdcheck(args = c("--as-cran"))
── R CMD build ─────────────────────────────────────────────────────────────────────────────────────
✔  checking for file ‘.../DESCRIPTION’ ...
─  preparing ‘amadeus’: (3.3s)
✔  checking DESCRIPTION meta-information ...
─  installing the package to process help pages
   Loading required namespace: amadeus
─  saving partial Rd database (5.2s)
✔  creating vignettes (5.8s)
─  checking for LF line-endings in source and make files and shell scripts
─  checking for empty or unneeded directories
   Removed empty directory ‘amadeus/inst/extdata/data_files’
   Removed empty directory ‘amadeus/inst/extdata/zip_files’
─  building ‘amadeus_1.0.3.tar.gz’
   
── R CMD check ─────────────────────────────────────────────────────────────────────────────────────
─  using log directory ‘/tmp/RtmpJyLNev/file27500a508e4c28/amadeus.Rcheck’
─  using R version 4.3.2 (2023-10-31)
─  using platform: x86_64-pc-linux-gnu (64-bit)
─  R was compiled by
       gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18)
       GNU Fortran (GCC) 8.5.0 20210514 (Red Hat 8.5.0-18)
─  running under: Rocky Linux 8.8 (Green Obsidian)
─  using session charset: UTF-8
─  using option ‘--as-cran’
✔  checking for file ‘amadeus/DESCRIPTION’
─  this is package ‘amadeus’ version ‘1.0.3’
─  package encoding: UTF-8
─  checking CRAN incoming feasibility ... [5s/62s] NOTE (1m 2.5s)
   Maintainer: ‘Kyle Messier <kyle.messier@nih.gov>’
   
   New submission
✔  checking package namespace information ...
✔  checking package dependencies (594ms)
✔  checking if this is a source package ...
✔  checking if there is a namespace
✔  checking for executable files (496ms)
✔  checking for hidden files and directories ...
✔  checking for portable file names ...
✔  checking for sufficient/correct file permissions ...
─  checking whether package ‘amadeus’ can be installed ... [95s/18s] OK (17.6s)
✔  checking installed package size ...
✔  checking package directory
✔  checking for future file timestamps ...
✔  checking ‘build’ directory ...
✔  checking DESCRIPTION meta-information (343ms)
✔  checking top-level files ...
✔  checking for left-over files ...
✔  checking index information (355ms)
✔  checking package subdirectories (360ms)
✔  checking R files for non-ASCII characters ...
✔  checking R files for syntax errors ...
✔  checking whether the package can be loaded (4.8s)
✔  checking whether the package can be loaded with stated dependencies (4.5s)
✔  checking whether the package can be unloaded cleanly (4.5s)
✔  checking whether the namespace can be loaded with stated dependencies (4.5s)
✔  checking whether the namespace can be unloaded cleanly (4.7s)
✔  checking loading without being on the library search path (4.8s)
✔  checking use of S3 registration (10.8s)
✔  checking dependencies in R code (5s)
✔  checking S3 generic/method consistency (4.9s)
✔  checking replacement functions (4.5s)
✔  checking foreign function calls (4.9s)
─  checking R code for possible problems ... [211s/25s] OK (25.1s)
✔  checking Rd files (6.1s)
✔  checking Rd metadata ...
✔  checking Rd line widths ...
✔  checking Rd cross-references ...
✔  checking for missing documentation entries (4.8s)
✔  checking for code/documentation mismatches (14.3s)
✔  checking Rd \usage sections (5.4s)
✔  checking Rd contents ...
✔  checking for unstated dependencies in examples (336ms)
    WARNING
   ‘qpdf’ is needed for checks on size reduction of PDFs
✔  checking installed files from ‘inst/doc’ ...
✔  checking files in ‘vignettes’ ...
─  checking examples ... [35s/31s] OK (31.4s)
✔  checking for unstated dependencies in vignettes (364ms)
✔  checking package vignettes in ‘inst/doc’
✔  checking re-building of vignette outputs (5.7s)
✔  checking PDF version of manual (2.7s)
N  checking HTML version of manual ...
   Skipping checking HTML validation: no command 'tidy' found
✔  checking for non-standard things in the check directory
✔  checking for detritus in the temp directory
   
   See
     ‘/tmp/RtmpJyLNev/file27500a508e4c28/amadeus.Rcheck/00check.log’
   for details.
   
   
── R CMD check results ────────────────────────────────────────────────────────── amadeus 1.0.3 ────
Duration: 3m 59.6s

❯ checking for unstated dependencies in examples ... OK
   WARNING
  ‘qpdf’ is needed for checks on size reduction of PDFs

❯ checking CRAN incoming feasibility ... [5s/62s] NOTE
  Maintainer: ‘Kyle Messier <kyle.messier@nih.gov>’
  
  New submission

❯ checking HTML version of manual ... NOTE
  Skipping checking HTML validation: no command 'tidy' found

0 errors ✔ | 1 warning ✖ | 2 notes ✖
> 

I was receiving these same NOTES and WARNINGS in local checks before our previous submission, which passed the automated tests, so I am more concerned about the spelling error and the URL verification.

Attached shows run of download_hms using NIEHS remote Windows desktop. I had to edit the curl command to be U://Desktop/bin/curl.exe ... because the virtual desktop does not have curl and I do not have permission to edit environmental variables. Besides this change, the download functions and download_run download data as expected.
Screenshot 2024-08-09 at 8 37 11 AM (2)

@mitchellmanware mitchellmanware merged commit 7c99607 into main Aug 13, 2024
10 checks passed
@mitchellmanware mitchellmanware deleted the cran-0801 branch August 16, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants