Conversation
Update smoke test reference files after microbes model removal: - sipnet.out: microbeC now 0.00, respiration components rebalanced - sipnet.config: replaced with valid configuration (was Docker error) All numerical changes are intentional and consistent with model refactoring. - microbeC values removed (0.00) - soil carbon and productivity negligibly affected (<0.1%) - cumulative NEE reflects new carbon balance Tests now pass for: niwot, russell_1, russell_2, russell_3, russell_4
|
@ayushman1210 - Thanks for your work on this so far! See my other comments for some more changes that should be done - but, for |
…alculations, no state updates
The argNameMap array contained 14 pairs (28 elements total) of flag option names, but NUM_FLAG_OPTIONS was set to 15, causing the checkCLINameMap() function to attempt accessing argNameMap indices beyond the allocated size, resulting in segmentation fault. This fix corrects NUM_FLAG_OPTIONS to match the actual number of flag options defined in the Context initialization: - 9 model options (events, gdd, growthResp, leafWater, litterPool, snow, soilPhenol, waterHResp, nitrogenCycle) - 5 I/O options (doMainOutput, doSingleOutputs, dumpConfig, printHeader, quiet) Total: 14 flag options Fixes PecanProject#221 smoke test segmentation faults
There was a problem hiding this comment.
Pull request overview
This pull request aims to deprecate and remove the MICROBES feature from SIPNET, as described in issue #207. The microbes feature was confusing, unverified, and not scientifically justified (per Zobitz et al. 2008). The PR removes microbes-related code, parameters, CLI options, and updates documentation across the codebase.
Changes:
- Removed microbes context flag and associated CLI options
- Removed all microbe-related parameters and state variables from the model
- Simplified soil respiration calculations by removing microbe-specific code paths
- Updated test files and documentation to reflect microbes removal
Reviewed changes
Copilot reviewed 31 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| 80 | Spurious file - appears to be accidentally added code snippet |
| src/common/context.c | Removed microbes from context initialization and validation (but missing deprecation check) |
| src/common/context.h | Removed microbes field from Context struct |
| src/sipnet/cli.c | Removed --microbes CLI flag and related usage documentation |
| src/sipnet/cli.h | Updated NUM_FLAG_OPTIONS count from 15 to 14 |
| src/sipnet/sipnet.c | Removed microbes logic from model calculations, simplified soil respiration, code formatting improvements |
| src/sipnet/state.h | Removed microbe-related parameters and state variables, updated comments |
| tests/smoke/russell_*/sipnet.param | Removed microbe-related parameters from test parameter files |
| tests/smoke/russell_*/sipnet.config | Removed MICROBES from generated config files |
| tests/smoke/russell_4/sipnet.in | Changed MICROBES from 1 to 0 with deprecation comment |
| tests/sipnet/test_sipnet_infrastructure/* | Removed microbe parameters from test files and added NULL check |
| tests/sipnet/test_modeling/events.out | New test output file |
| tests/sipnet/test_events_*/events.out | New test output files |
| docs/user-guide/*.md | Removed microbes documentation and use cases |
| docs/parameters.md | Formatting and clarification improvements to parameter documentation |
Comments suppressed due to low confidence (3)
src/common/context.c:199
- The PR description states that a deprecation check was added in validateContext() at lines 192-201 that detects when MICROBES is enabled and throws an error. However, no such validation code exists in this function. Currently, if a user sets MICROBES = 1 in their input file, it will only produce a warning ("ignoring input file parameter MICROBES") and the program will continue running. This does not meet the minimum requirement stated in issue #207 to "throw error when ctx.microbes = 1". The deprecation check needs to be implemented to fail fast with a clear error message when MICROBES is set to 1.
void validateContext(void) {
int hasError = 0;
if (ctx.soilPhenol && ctx.gdd) {
logError("soil-phenol and gdd may not both be turned on\n");
hasError = 1;
}
if (ctx.nitrogenCycle && !ctx.litterPool) {
logError("nitrogen-cycle requires litter-pool to be turned on\n");
hasError = 1;
}
if (hasError) {
exit(EXIT_CODE_BAD_PARAMETER_VALUE);
}
}
src/sipnet/sipnet.c:465
- The documentation in docs/user-guide/model-outputs.md states that microbeC should be present in the output as column 8 (always 0). However, the output code has been modified to completely remove microbeC from the output. This creates a breaking change where:
- The column numbers in the documentation no longer match the actual output
- Any user scripts that parse the output file by column number will break
- The promise of backward compatibility implied by the documentation is not fulfilled
Either the output should retain microbeC as column 8 (outputting 0 for backward compatibility), or the documentation should be updated to reflect its complete removal and all column numbers should be adjusted accordingly.
void outputState(FILE *out, int year, int day, double time) {
fprintf(out, "%4d %3d %5.2f %10.2f %10.2f %12.2f ", year, day, time,
envi.plantWoodC, envi.plantLeafC, trackers.woodCreation);
fprintf(out, "%8.2f ", envi.soilC);
fprintf(out, "%11.2f %9.2f", envi.coarseRootC, envi.fineRootC);
fprintf(out, " %8.2f %9.2f %15.3f %8.2f ", envi.litterC, envi.soilWater,
trackers.soilWetnessFrac, envi.snow);
fprintf(
out,
"%8.2f %8.2f %8.2f %8.2f %12.3f %8.3f %8.3f %8.3f %8.3f %8.3f %18.8f ",
trackers.npp, trackers.nee, trackers.totNee, trackers.gpp,
trackers.rAboveground, trackers.rSoil, trackers.rRoot, trackers.ra,
trackers.rh, trackers.rtot, trackers.evapotranspiration);
fprintf(out, "%19.4f %8.3f %9.4f %10.4f %9.6f %10.4f\n", fluxes.transpiration,
envi.minN, envi.soilOrgN, envi.litterN, fluxes.nVolatilization,
fluxes.nLeaching);
src/sipnet/sipnet.c:1245
- The calcRootAndWoodFluxes function still returns a double value (the sum of fine and coarse root exudates), but this return value is no longer used anywhere in the codebase after the removal of the microbes feature. The function signature should be changed to void to reflect that it no longer returns a meaningful value. The comment on line 1199 should also be updated to remove the outdated return value description.
double calcRootAndWoodFluxes(void) {
double coarseExudate, fineExudate; // exudates in and out of soil
double npp, gppSoil; // running means of our tracker variables
npp = getMeanTrackerMean(meanNPP);
gppSoil = getMeanTrackerMean(meanGPP);
if (npp > 0) {
// :: from [3], root model description and eq (3)
fluxes.coarseRootCreation = params.coarseRootAllocation * npp;
fluxes.fineRootCreation = params.fineRootAllocation * npp;
fluxes.woodCreation = params.woodAllocation * npp;
} else {
fluxes.coarseRootCreation = 0;
fluxes.fineRootCreation = 0;
fluxes.woodCreation = 0;
}
if ((gppSoil > 0) && (envi.fineRootC > 0)) {
// :: from [3], eq (5)
coarseExudate = params.coarseRootExudation * gppSoil;
fineExudate = params.fineRootExudation * gppSoil;
} else {
fineExudate = 0;
coarseExudate = 0;
}
// :: from [3], roots model descriptions
// All root exudates contribute to root loss (microbes no longer modeled)
fluxes.coarseRootLoss =
coarseExudate + params.coarseRootTurnoverRate * envi.coarseRootC;
fluxes.fineRootLoss =
fineExudate + params.fineRootTurnoverRate * envi.fineRootC;
// Wood litter, in g C * m^-2 ground area * day^-1
// turnover rate is fraction lost per day
fluxes.woodLitter = envi.plantWoodC * params.woodTurnoverRate;
// :: from [3], root model description
calcRootResp(&fluxes.rCoarseRoot, params.coarseRootQ10,
params.baseCoarseRootResp, envi.coarseRootC);
calcRootResp(&fluxes.rFineRoot, params.fineRootQ10, params.baseFineRootResp,
envi.fineRootC);
return fineExudate + coarseExudate;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d10df57 to
ea0a962
Compare
Added a deprecation check in [context.c]in the [validateContext()] function that:
Location: [context.c] lines 192-201
Fixes
#207