-
Notifications
You must be signed in to change notification settings - Fork 311
Replace manual unit conversions with ud_convert() for consistency across models #3719
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
59 commits
Select commit
Hold shift + click to select a range
5599e09
Replace manual unit conversions with ud_convert()
ayushman1210 2fc0106
Update models/stics/R/met2model.STICS.R
ayushman1210 c425ca7
Update models/sipnet/R/write_restart.SIPNET.R
ayushman1210 747277a
Merge branch 'develop' into fix
ayushman1210 c29e56f
fixes done as per maintainer suggestion
ayushman1210 a2d79d5
Merge branch 'fix' of https://github.com/ayushman1210/pecan into fix
ayushman1210 8e2b0a4
Merge branch 'develop' into fix
ayushman1210 aa8f2d4
Update models/dalec/R/model2netcdf.DALEC.R
ayushman1210 e6ad156
Update models/dalec/R/model2netcdf.DALEC.R
ayushman1210 0c2bb74
Update models/gday/R/model2netcdf.GDAY.R
ayushman1210 1aeddf7
Update tests/testthat/test_model_conversions.R
ayushman1210 570b899
Update tests/testthat/test_model_conversions.R
ayushman1210 9970af2
Update tests/testthat/test_model_conversions.R
ayushman1210 476de3d
Update tests/testthat/test_model_conversions.R
ayushman1210 2fdc5d9
Merge branch 'develop' into fix
infotroph 46773d1
Merge branch 'develop' into fix
ayushman1210 20bd418
changes as suggested by the maintainer
ayushman1210 adecf54
suggested changes
ayushman1210 6ecf394
Merge branch 'develop' into fix
ayushman1210 10950c9
Merge branch 'develop' into fix
ayushman1210 a21b66f
Merge branch 'develop' into fix
ayushman1210 a7d584a
Merge branch 'develop' into fix
ayushman1210 f78452c
Merge branch 'develop' into fix
ayushman1210 c7786e6
Merge branch 'develop' into fix
ayushman1210 c84dc76
Merge branch 'develop' into fix
ayushman1210 3b1d45d
Merge branch 'develop' into fix
infotroph ae48649
Update models/fates/R/write.configs.FATES.R
ayushman1210 4d9ca18
Update models/fates/R/write.configs.FATES.R
ayushman1210 b63051f
Update models/sipnet/R/model2netcdf.SIPNET.R
ayushman1210 014e58f
Update models/fates/R/write.configs.FATES.R
ayushman1210 d0f49ee
Update models/fates/R/write.configs.FATES.R
ayushman1210 73db407
Merge branch 'develop' into fix
ayushman1210 785d156
removed testthat.R updated model2netcdf.SIPNET.R updated model2netcdf…
ayushman1210 de366f6
Fix: Skip Docker push on forks to avoid ghcr.io authentication errors
ayushman1210 ec3c96c
Add GDAY unit conversion tests and clean up redundant test files
ayushman1210 6ac287a
Add unit tests for SIPNET model2netcdf unit conversion changes
ayushman1210 136db35
Clarify GDAY unit conversion: daily Mg/ha/day not per-year
ayushman1210 05f6735
Merge branch 'develop' into fix
ayushman1210 174728b
Update models/sipnet/tests/testthat/test.model2netcdf.SIPNET.R
ayushman1210 bef3eb8
Address maintainer feedback: revert workflow changes, fix GDAY refere…
ayushman1210 12539c6
Update SIPNET test to exercise model2netcdf.SIPNET() directly, addres…
ayushman1210 cfa5d1d
Update GDAY test to exercise model2netcdf.GDAY() directly, addressing…
ayushman1210 4d2a612
Fix undefined THA_2_KG_M2 in GDAY model2netcdf by using ud_convert
ayushman1210 b8d8130
Fix GDAY unit conversion test to use manual calculation instead of pr…
ayushman1210 4427363
fixed the ci-failure
ayushman1210 4ba11dc
Merge branch 'develop' into fix
ayushman1210 b982d92
Merge branch 'develop' into fix
ayushman1210 df85039
Update models/gday/tests/testthat/test.model2netcdf.GDAY.R
ayushman1210 d7b2434
Update models/gday/R/model2netcdf.GDAY.R
ayushman1210 110ffef
Update models/gday/R/model2netcdf.GDAY.R
ayushman1210 dbeaf47
Merge branch 'develop' into fix
ayushman1210 3502db0
Standardize unit conversions and fix tests for PR #3719
ayushman1210 d7e5ef3
Add TODO comment for woodCreation in SIPNET
ayushman1210 eab202c
Update models/sipnet/R/model2netcdf.SIPNET.R
infotroph 68421f3
update deps
infotroph da2fab8
Merge branch 'develop' into fix
ayushman1210 bdd4a76
hack around test failure until full V2 support replaces this
infotroph b280b19
more conditionals
infotroph b972c26
Merge branch 'develop' into fix
infotroph 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
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
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
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,49 @@ | ||
| # Test script to validate unit conversion changes | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
| # This verifies that ud_convert works with the new unit strings | ||
|
|
||
| library(PEcAn.utils) | ||
|
|
||
| cat("Testing unit string validity with UDUNITS2...\n\n") | ||
|
|
||
| # Test cases from the modified files | ||
| test_conversions <- list( | ||
| list(value = 100, u1 = "g/m2", u2 = "kg/m2", expected = 0.1, desc = "DALEC/SIPNET pool: g/m2 to kg/m2"), | ||
| list(value = 86400, u1 = "g/m2/d", u2 = "kg/m2/s", expected = 0.001, desc = "DALEC/SIPNET flux: g/m2/d to kg/m2/s"), | ||
| list(value = 10, u1 = "Mg/ha", u2 = "kg/m2", expected = 1, desc = "GDAY: Mg/ha to kg/m2"), | ||
| list(value = 1000, u1 = "J/mol", u2 = "kJ/mol", expected = 1, desc = "FATES: J/mol to kJ/mol") | ||
| ) | ||
|
|
||
| results <- list() | ||
| for (i in seq_along(test_conversions)) { | ||
| test <- test_conversions[[i]] | ||
| tryCatch({ | ||
| result <- ud_convert(test$value, test$u1, test$u2) | ||
| passed <- abs(result - test$expected) < 1e-10 | ||
| results[[i]] <- list( | ||
| desc = test$desc, | ||
| passed = passed, | ||
| result = result, | ||
| expected = test$expected | ||
| ) | ||
| cat(sprintf("[%s] %s\n", if(passed) "PASS" else "FAIL", test$desc)) | ||
| if (!passed) { | ||
| cat(sprintf(" Expected: %f, Got: %f\n", test$expected, result)) | ||
| } | ||
| }, error = function(e) { | ||
| results[[i]] <<- list(desc = test$desc, passed = FALSE, error = e$message) | ||
| cat(sprintf("[FAIL] %s\n", test$desc)) | ||
| cat(sprintf(" Error: %s\n", e$message)) | ||
| }) | ||
| } | ||
|
|
||
| cat("\n") | ||
| passed_count <- sum(sapply(results, function(x) x$passed)) | ||
| total_count <- length(results) | ||
| cat(sprintf("Summary: %d/%d tests passed\n", passed_count, total_count)) | ||
|
|
||
| if (passed_count < total_count) { | ||
| cat("\nFAILED TESTS DETECTED - These must be fixed before proceeding\n") | ||
| quit(status = 1) | ||
| } else { | ||
| cat("\nAll unit strings are valid!\n") | ||
| } | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
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,4 @@ | ||
| # Regression tests for unit conversion refactoring | ||
| # Run with: testthat::test_dir("tests/testthat", filter = "test_model_conversions") | ||
|
|
||
| source("test_model_conversions.R") | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
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,71 @@ | ||
| context("Unit Conversion Refactoring Tests") | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # These tests validate the refactoring to use PEcAn.utils::ud_convert() | ||
| # for consistent unit conversions across SIPNET, DALEC, GDAY, and FATES models. | ||
|
|
||
| test_that("UDUNITS2 recognizes new unit strings", { | ||
| library(PEcAn.utils) | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Pool conversions (mass only) | ||
| expect_equal(ud_convert(100, "g/m2", "kg/m2"), 0.1) | ||
| expect_equal(ud_convert(1000, "g/m2", "kg/m2"), 1.0) | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Flux conversions (mass and time) | ||
| expect_equal(ud_convert(86400, "g/m2/d", "kg/m2/s"), 0.001, tolerance = 1e-10) | ||
|
|
||
| # Large-scale conversions (GDAY) | ||
| expect_equal(ud_convert(10, "Mg/ha", "kg/m2"), 1.0) | ||
|
|
||
| # Energy conversions (FATES) | ||
| expect_equal(ud_convert(1000, "J/mol", "kJ/mol"), 1.0) | ||
| }) | ||
|
|
||
| test_that("SIPNET flux conversions produce realistic values", { | ||
| library(PEcAn.utils) | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # GPP: ~5 g C/m2/d → kg/m2/s (should be very small) | ||
| gpp_result <- ud_convert(5, "g/m2/d", "kg/m2/s") | ||
| expect_true(gpp_result > 0) | ||
| expect_true(gpp_result < 5) # Should be much smaller | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Pool: 1000 g C/m2 → 1 kg/m2 | ||
| wood_result <- ud_convert(1000, "g/m2", "kg/m2") | ||
| expect_equal(wood_result, 1.0) | ||
| }) | ||
|
|
||
| test_that("DALEC flux conversions are mathematically sound", { | ||
| library(PEcAn.utils) | ||
|
|
||
| # Autotrophic respiration: 5 g C/m2/d → kg/m2/s | ||
| ar_result <- ud_convert(5, "g/m2/d", "kg/m2/s") | ||
| expect_true(ar_result > 0) | ||
| expect_true(ar_result < 5) | ||
|
|
||
| # Leaf carbon: 200 g C/m2 → 0.2 kg/m2 | ||
| leaf_result <- ud_convert(200, "g/m2", "kg/m2") | ||
| expect_equal(leaf_result, 0.2) | ||
| }) | ||
|
|
||
| test_that("GDAY conversions from Mg/ha to kg/m2 work correctly", { | ||
| library(PEcAn.utils) | ||
|
|
||
| # 5 Mg/ha = 0.5 kg/m2 | ||
| stem_result <- ud_convert(5, "Mg/ha", "kg/m2") | ||
| expect_equal(stem_result, 0.5) | ||
|
|
||
| # 50 Mg/ha = 5 kg/m2 | ||
| soil_result <- ud_convert(50, "Mg/ha", "kg/m2") | ||
| expect_equal(soil_result, 5.0) | ||
| }) | ||
|
|
||
| test_that("No 'C' prefix in unit strings (UDUNITS2 requirement)", { | ||
| library(PEcAn.utils) | ||
|
|
||
| # These should NOT work (they include 'C' for carbon) | ||
| expect_error(ud_convert(100, "gC/m2", "kgC/m2")) | ||
| expect_error(ud_convert(86400, "gC/m2/d", "kgC/m2/s")) | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
|
|
||
| # But these (without 'C') should work | ||
| expect_equal(ud_convert(100, "g/m2", "kg/m2"), 0.1) | ||
| expect_equal(ud_convert(86400, "g/m2/d", "kg/m2/s"), 0.001, tolerance = 1e-10) | ||
|
ayushman1210 marked this conversation as resolved.
Outdated
|
||
| }) | ||
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.
Uh oh!
There was an error while loading. Please reload this page.