NIEHS / amadeus

https://niehs.github.io/amadeus/
Other
7 stars 1 forks source link

Mm copy functions #15

Closed mitchellmanware closed 9 months ago

mitchellmanware commented 9 months ago
  1. Migrate NARR, GEOS-CF, HMS, GMTED, and SEDAC population functions and tests from beethoven
  2. Merge with current process.R and calculate_covariates.R files
  3. Add spatial subset test data sets
  4. Update README.md
  5. download_noaa_hms_smoke_data > download_hms_data (reflected in man/ and all tests)
  6. Utilize base R strsplit() in the extract_urls() download support function.
    • Replaced stringr dependency with stringi for stringi::stri_pad() in process_narr()
codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.76%. Comparing base (37ba1fe) to head (0ff1926).

:exclamation: Current head 0ff1926 differs from pull request most recent head 37f8aea. Consider uploading reports for the commit 37f8aea to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #15 +/- ## ========================================== + Coverage 94.45% 95.76% +1.30% ========================================== Files 6 6 Lines 2182 2974 +792 ========================================== + Hits 2061 2848 +787 - Misses 121 126 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mitchellmanware commented 9 months ago
Screenshot 2024-02-21 at 2 18 50 PM

testthat output shows that all tests are passing, but the workflow fails when uploading the coverage to Codecov.

kyle-messier commented 9 months ago

@mitchellmanware I'm not sure what to do. I would suggest first updating your branch with main as I am seeing this warning from GitHub (i.e. your branch is both ahead and behind main). Interestingly the build checks and test-coverage appeared to take 3-4x longer than when they ran for me.

mitchellmanware commented 9 months ago

Pulled in the updates so we will see if that makes a difference.

It is weird how the different Ubuntu versions fail the build check seemingly randomly. In this commit oldrel-1 was the only to fail, but later it is the only one to pass.

I assume it stems from whatever is causing the testthat upload to fail, but changing the Sys.sleep() values in both the test_download_functions() function and in the test-downoad_functions.R tests causes unpredictable pass/failure.

The unit tests run very quick locally, so also unsure about the running time in the GitHub runners. @Spatiotemporal-Exposures-and-Toxicology

mitchellmanware commented 9 months ago

test-coverage finally passes but still takes 52 minutes. Failure for ubuntu-latest (release) caused by same problem. There might be system-specific differences for the URL responses, which could be why the different sleep times cause certain check to pass and fail.

I will keep looking into it, but at least tests are passing.

mitchellmanware commented 9 months ago

Exploring replacing httr::HEAD() and httr::GET() with httr2::request() > req_perform(). httr2 is a replacement to the httr package, so I will see if quicker HTTP response times fix the test-coverage and ubuntu check failures.

> microbenchmark(
+   request(urls[1]) %>% req_perform(),
+   httr::HEAD(urls[1]),
+   times = 10,
+   unit = "seconds"
+ )
Unit: seconds
                               expr       min        lq      mean    median         uq        max neval
 request(urls[1]) %>% req_perform() 0.2373555 0.2524261 0.3734844 0.3562834  0.4702028  0.5638149    10
                httr::HEAD(urls[1]) 5.4498386 8.4269201 9.5435405 9.0156858 11.0656759 16.0476204    10
> 
mitchellmanware commented 9 months ago

Somehow the httr2 package resulted in three hour run times for the R CMD checks. Not sure how this is possible but I will revert to previous version with httr.

mitchellmanware commented 9 months ago

This thread describes issues using .RDS files in testthat tests. I deleted the sites_NC.rds file I was using for testing and matched the

  ncp <- data.frame(lon = -78.8277, lat = 35.95013)
  ncp$site_id <- "3799900018810101"

testing locations used in other covariate calculation unit tests.

Also renamed geos test data folders as aqc and chm to limit characters in file paths. Updated calc_geos detects collection from .nc file paths so full collection name is not needed.

mitchellmanware commented 9 months ago

I am now getting test failures for process_geos_collection, which is a simple string splitting and selection function. I am going to close PR and create new branch from most recent merge from main.