-
Notifications
You must be signed in to change notification settings - Fork 313
Add standard_result S3 class for unified preprocessing return values. #4015
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
Open
shreyannandanwar
wants to merge
3
commits into
PecanProject:develop
Choose a base branch
from
shreyannandanwar:module1
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,13 @@ | ||
| # Generated by roxygen2: do not edit by hand | ||
|
|
||
| export(do_conversions) | ||
| export(insert_preprocess_result) | ||
| export(preprocess_result) | ||
| export(run.write.configs) | ||
| export(runModule.get.trait.data) | ||
| export(runModule.run.write.configs) | ||
| export(runModule_start_model_runs) | ||
| export(start_model_runs) | ||
| export(validate_preprocess_result) | ||
| importFrom(dplyr,"%>%") | ||
| importFrom(rlang,"%||%") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| #' Preprocessing Result Constructor | ||
| #' | ||
| #' Uniform return type for all preprocessing pipeline functions. | ||
| #' Returns a named list that can be inserted directly into a PEcAn settings object. | ||
| #' Disk-only for now; memory-primary path can be added later without breaking | ||
| #' the interface. | ||
| #' | ||
| #' @param file Character. Path to the generated file. | ||
| #' @param format Character. Format identifier (e.g., "CFmet", "ED2met"). | ||
| #' @param mimetype Character. MIME type of the file. | ||
| #' @param dbfile.id Integer. BETY dbfile ID, if available. NULL if dbparms = NULL. | ||
| #' @param start_date POSIXct. Start date of the data period. | ||
| #' @param end_date POSIXct. End date of the data period. | ||
| #' @param source Character. Data source identifier (e.g., "Ameriflux", "CRUNCEP"). | ||
| #' | ||
| #' @return A named list with class attribute "preprocess_result" for potential | ||
| #' future subclassing, but behaves as a plain list for now. | ||
| #' | ||
| #' @examples | ||
| #' result <- preprocess_result( | ||
| #' file = "/data/flux/US-NR1.2004.nc", | ||
| #' format = "CFmet", | ||
| #' mimetype = "application/x-netcdf", | ||
| #' dbfile.id = 12345, | ||
| #' start_date = as.POSIXct("2004-01-01"), | ||
| #' end_date = as.POSIXct("2004-12-31"), | ||
| #' source = "Ameriflux" | ||
| #' ) | ||
| #' | ||
| #' @md | ||
| #' @export | ||
| preprocess_result <- function(file, format, mimetype, dbfile.id = NULL, | ||
| start_date = NULL, end_date = NULL, source = NULL) { | ||
| # Validation - using PEcAn.logger::logger.severe() for consistency with codebase | ||
| if (!is.character(file) || length(file) != 1) { | ||
| PEcAn.logger::logger.severe("file must be a single character path") | ||
| } | ||
| if (!file.exists(file)) { | ||
| PEcAn.logger::logger.warn("file does not exist:", file) | ||
| } | ||
| if (!is.character(format) || length(format) != 1) { | ||
| PEcAn.logger::logger.severe("format must be a single character string") | ||
| } | ||
| if (!is.character(mimetype) || length(mimetype) != 1) { | ||
| PEcAn.logger::logger.severe("mimetype must be a single character string") | ||
| } | ||
| if (!is.null(dbfile.id) && (!is.numeric(dbfile.id) || length(dbfile.id) != 1)) { | ||
| PEcAn.logger::logger.severe("dbfile.id must be a single integer or NULL") | ||
| } | ||
|
|
||
| result <- list( | ||
| file = file, | ||
| format = format, | ||
| mimetype = mimetype, | ||
| dbfile.id = dbfile.id, | ||
| start_date = start_date, | ||
| end_date = end_date, | ||
| source = source | ||
| ) | ||
|
|
||
| # Class attribute for future subclassing, but no S3 methods yet | ||
| class(result) <- c("preprocess_result", "list") | ||
|
|
||
| result | ||
| } | ||
|
|
||
| #' Insert Preprocess Result into Settings | ||
| #' | ||
| #' Takes a preprocess_result (or list of them) and patches it into the | ||
| #' appropriate location in a PEcAn settings object. | ||
| #' | ||
| #' @param settings List. PEcAn settings object. | ||
| #' @param result preprocess_result or list of preprocess_result. | ||
| #' @param input.type Character. Input type identifier, e.g., "met", "soil". | ||
| #' | ||
| #' @return Updated settings object. | ||
| #' | ||
| #' @md | ||
| #' @export | ||
| insert_preprocess_result <- function(settings, result, input.type) { | ||
| if (!is.list(result)) { | ||
| PEcAn.logger::logger.severe("result must be a list or preprocess_result") | ||
| } | ||
|
|
||
| # Handle single result or list of results | ||
| if (inherits(result, "preprocess_result")) { | ||
| result <- list(result) | ||
| } | ||
|
|
||
| # Ensure path exists in settings | ||
| if (is.null(settings$run$inputs)) { | ||
| settings$run$inputs <- list() | ||
| } | ||
| if (is.null(settings$run$inputs[[input.type]])) { | ||
| settings$run$inputs[[input.type]] <- list() | ||
| } | ||
|
|
||
| # Insert results | ||
| settings$run$inputs[[input.type]]$path <- sapply(result, `[[`, "file") | ||
| settings$run$inputs[[input.type]]$format <- result[[1]]$format | ||
| settings$run$inputs[[input.type]]$mimetype <- result[[1]]$mimetype | ||
|
|
||
| if (!is.null(result[[1]]$dbfile.id)) { | ||
| settings$run$inputs[[input.type]]$dbfile.id <- sapply(result, `[[`, "dbfile.id") | ||
| } | ||
|
|
||
| settings | ||
| } | ||
|
|
||
| #' Validate Preprocess Result | ||
| #' | ||
| #' Check that a preprocess_result has all required fields and valid values. | ||
| #' Useful for parity tests and pipeline validation. | ||
| #' | ||
| #' @param result List. Object to validate. | ||
| #' @param require.file.exists Logical. Whether to check file existence. | ||
| #' | ||
| #' @return Logical. TRUE if valid, throws error otherwise. | ||
| #' | ||
| #' @md | ||
| #' @export | ||
| validate_preprocess_result <- function(result, require.file.exists = TRUE) { | ||
| required_fields <- c("file", "format", "mimetype") | ||
|
|
||
| for (field in required_fields) { | ||
| if (is.null(result[[field]])) { | ||
| PEcAn.logger::logger.severe("Missing required field:", field) | ||
| } | ||
| } | ||
|
|
||
| if (!is.character(result$file) || length(result$file) != 1) { | ||
| PEcAn.logger::logger.severe("file must be a single character path") | ||
| } | ||
|
|
||
| if (require.file.exists && !file.exists(result$file)) { | ||
| PEcAn.logger::logger.severe("file does not exist:", result$file) | ||
| } | ||
|
|
||
| TRUE | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
|
|
||
| context("Preprocess Result Constructor") | ||
|
|
||
| test_that("preprocess_result creates valid object", { | ||
| tmpfile <- tempfile(fileext = ".nc") | ||
| file.create(tmpfile) | ||
| on.exit(unlink(tmpfile)) | ||
|
|
||
| result <- preprocess_result( | ||
| file = tmpfile, | ||
| format = "CFmet", | ||
| mimetype = "application/x-netcdf", | ||
| dbfile.id = 12345, | ||
| start_date = as.POSIXct("2004-01-01"), | ||
| end_date = as.POSIXct("2004-12-31"), | ||
| source = "Ameriflux" | ||
| ) | ||
|
|
||
| expect_type(result, "list") | ||
| expect_equal(result$file, tmpfile) | ||
| expect_equal(result$format, "CFmet") | ||
| expect_equal(result$dbfile.id, 12345) | ||
| expect_s3_class(result, "preprocess_result") | ||
| expect_s3_class(result, "list") | ||
| }) | ||
|
|
||
| test_that("preprocess_result validates inputs with logger.severe", { | ||
| # logger.severe stops execution, so we expect error | ||
| expect_error( | ||
| preprocess_result(file = 123, format = "CFmet", mimetype = "application/x-netcdf"), | ||
| "file must be a single character path" | ||
| ) | ||
|
|
||
| expect_error( | ||
| preprocess_result(file = "foo.nc", format = NULL, mimetype = "application/x-netcdf"), | ||
| "format must be a single character string" | ||
| ) | ||
| }) | ||
|
|
||
| test_that("preprocess_result accepts missing files", { | ||
| result <- preprocess_result( | ||
| file = "/nonexistent/file.nc", | ||
| format = "CFmet", | ||
| mimetype = "application/x-netcdf" | ||
| ) | ||
|
|
||
| expect_equal(result$file, "/nonexistent/file.nc") | ||
| }) | ||
|
|
||
| test_that("insert_preprocess_result patches settings correctly", { | ||
| settings <- list(run = list(inputs = list())) | ||
| tmpfile <- tempfile(fileext = ".nc") | ||
| file.create(tmpfile) | ||
| on.exit(unlink(tmpfile)) | ||
|
|
||
| result <- preprocess_result( | ||
| file = tmpfile, | ||
| format = "CFmet", | ||
| mimetype = "application/x-netcdf" | ||
| ) | ||
|
|
||
| updated <- insert_preprocess_result(settings, result, "met") | ||
|
|
||
| expect_equal(updated$run$inputs$met$path, tmpfile) | ||
| expect_equal(updated$run$inputs$met$format, "CFmet") | ||
| }) | ||
|
|
||
| test_that("insert_preprocess_result handles multiple results", { | ||
| settings <- list(run = list(inputs = list())) | ||
| tmpfile1 <- tempfile(fileext = ".nc") | ||
| tmpfile2 <- tempfile(fileext = ".nc") | ||
| file.create(tmpfile1) | ||
| file.create(tmpfile2) | ||
| on.exit({ | ||
| unlink(tmpfile1) | ||
| unlink(tmpfile2) | ||
| }) | ||
|
|
||
| results <- list( | ||
| preprocess_result(file = tmpfile1, format = "CFmet", mimetype = "application/x-netcdf"), | ||
| preprocess_result(file = tmpfile2, format = "CFmet", mimetype = "application/x-netcdf") | ||
| ) | ||
|
|
||
| updated <- insert_preprocess_result(settings, results, "met") | ||
|
|
||
| expect_equal(updated$run$inputs$met$path, c(tmpfile1, tmpfile2)) | ||
| }) | ||
|
|
||
| test_that("validate_preprocess_result catches invalid results", { | ||
| expect_error( | ||
| validate_preprocess_result(list(format = "CFmet")), | ||
| "Missing required field: file" | ||
| ) | ||
|
|
||
| tmpfile <- tempfile() | ||
| file.create(tmpfile) | ||
| on.exit(unlink(tmpfile)) | ||
|
|
||
| valid <- preprocess_result(file = tmpfile, format = "CFmet", mimetype = "application/x-netcdf") | ||
| expect_true(validate_preprocess_result(valid)) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change was introduced by regenerating doc with roxygen2 8.x, which automatically replaces
RoxygenNotewithConfig/roxygen2/versioninDESCRIPTION.However, the current CI appears to rely on the presence of
RoxygenNote.Since this PR is not intended to migrate PEcAn to roxygen2 8, would it make sense to keep the existing
RoxygenNotefield for now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change was introduced when I regenerated documentation with roxygen2 8.x, which automatically replaces
RoxygenNotewithConfig/roxygen2/versioninDESCRIPTION.From the check output, it also looks like the current PEcAn tooling/CI still expects the existing
RoxygenNotefield.Since this PR isn't intended to migrate PEcAn's documentation tooling or roxygen version, would you prefer that I revert the
DESCRIPTIONchange and keep the existingRoxygenNoteentry for now?