Skip to content

Testing Infrastructure and Minor Function Fixes (Pre-Release 1.8.3 Review)#97

Merged
mariadlor merged 2 commits intoSantanderMetGroup:develfrom
mariadlor:devel
Aug 4, 2025
Merged

Testing Infrastructure and Minor Function Fixes (Pre-Release 1.8.3 Review)#97
mariadlor merged 2 commits intoSantanderMetGroup:develfrom
mariadlor:devel

Conversation

@mariadlor
Copy link
Copy Markdown
Collaborator

This PR proposes some changes to the loadeR package, including the addition of testing infrastructure and minor function fixes. The implemented changes are explained below, followed by the requested reviews. This review is intended as a final check before releasing version 1.8.3 of the package.

Summary of Changes

1. Function deaccum

Replaced:

deaccum <- function(x, t.ranges, dff)

With

deaccum <- function(x, t.ranges, dff = TRUE)

Reason:
This change was made to avoid an error when calling deaccum via apply() from within the dictionaryTransform() function. In the original implementation, deaccum required three arguments (x, t.ranges, dff), but only two (x, t.ranges) were passed via apply() in the dictionaryTransform() function. By setting dff = TRUE as the default, the function can now be safely used in this context without triggering argument mismatch errors.

2. Function loadDecadalForecast: Call to getMemberDomain

Replaced:

memberRangeList <- getMemberDomain(grid, dataset, members)

With:

memberRangeList <- getMemberDomain(grid, members)

Reason:
The function getMemberDomain() does not accept a dataset argument. Passing it caused an error when executing loadDecadalForecast(). The function call was corrected to match the actual signature of getMemberDomain(grid, members, continuous = FALSE).

3. Function makeAggregatedDataset: Filtering of NetCDF files per variable

Replaced:

varfile <- grep(vars[ind[i]], lf, value = TRUE)

With:

varfile <- lf[grep(vars[ind[i]], basename(lf))]

Reason:
The previous line searched for the variable name in the full file path (lf), which caused false matches when the variable name was present elsewhere in the path (e.g., in directory names). This led to incorrect files being selected for aggregation. The fix applies the match only to the file name using basename(lf), but still returns the full path of the matching files. This ensures accurate file selection while preserving the complete paths needed for processing.

4. Tests (New)

A testing structure has been added under the tests/testthat/ directory:

  • For each R script in the R/ directory, there is a corresponding test file in tests/testthat/.
  • Input data files required by the tests (e.g., .cdl and .txt files) are stored in tests/testthat/testdata/.
  • The script tests/testthat/setup.R is responsible for dynamically generating temporary NetCDF files (.nc) from the .cdl definitions. These files are created in the system's temporary directory (tempdir()) and automatically deleted once the test session ends (detailed instructions for running the test suite and checking coverage are available in the README.md).

5. README.md

Added clear instructions on how to create the test environment using Conda, and provided steps to:

  • Run the test suite using devtools::test(): this should execute all test files and complete successfully without errors or failures.
  • Compute test coverage using covr::package_coverage(): after some execution time, this should return a coverage report showing that approximately 80% of the code is covered in total, and also provides coverage statistics for each individual file in the R/ directory.
  • Develop the package locally without installing it with devtools::load_all().

Review Request

  • Review the new README.md section on testing and verify that the instructions are clear and complete.
  • Follow the setup and execution steps in README.md to run the tests locally and confirm that they work as expected.
  • Review the functional changes introduced in the deaccum(), loadDecadalForecast(), and makeAggregatedDataset() functions.

@mariadlor mariadlor requested review from jbedia and miturbide July 24, 2025 16:01
@mariadlor mariadlor self-assigned this Jul 24, 2025
Copy link
Copy Markdown
Member

@jbedia jbedia left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the fixes and adding the test units. Everything looks good to me!

@mariadlor mariadlor merged commit 813bd3f into SantanderMetGroup:devel Aug 4, 2025
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.

2 participants