Development towards v8.1.0 #418
Draft
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 73.22% 73.53% +0.30%
==========================================
Files 20 20
Lines 6025 6326 +301
==========================================
+ Hits 4412 4652 +240
- Misses 1613 1674 +61 ☔ View full report in Codecov by Sentry. |
src/filefuncs.c:437:23: error: Although the value stored to 'r' is used in the enclosing expression, the value is never actually read from 'r' [clang-analyzer-deadcode.DeadStores,-warnings-as-errors]
437 | if (0 != (r = mkdir(errstr, 0777))) {
src/SW_SoilWater.c:219:5: error: Value stored to 'k1' is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors]
219 | k1 = 3.174603e-08; // 0 < k1 = 0.2 / (b - a) < inf
src/SW_Flow.c:668:5: warning: Value stored to 'peti' is never read [clang-analyzer-deadcode.DeadStores]
668 | peti -= surface_evap_standingWater_rate;
src/SW_Flow_lib.c:2259:9: warning: Value stored to 'fusion_pool_init' is never read [clang-analyzer-deadcode.DeadStores]
2259 | fusion_pool_init = swTRUE;
src/SW_Weather.c:310:17: warning: Value stored to 'currMonDay' is never read [clang-analyzer-deadcode.DeadStores]
310 | currMonDay = SW_MISSING;
src/SW_Output_get_functions.c:663:5: warning: Value stored to 'i' is never read [clang-analyzer-deadcode.DeadStores]
663 | i = (IntU) pd; // silence `-Wunused-parameter`
src/SW_netCDF.c:2782:31: error: The right operand of '<' is a garbage value due to array index out of bounds [clang-analyzer-core.UndefinedBinaryOperatorResult,-warnings-as-errors]
2782 | for (index = 0; index < numVarAtts[dimNum]; index++) {
src/SW_Flow_lib.c:2231:29: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
2231 | temp = oldavgLyrTemp[0] + avgLyrTemp[0] + shParam + nlyrs + vwc[0] +
include/SW_Control.h:35:6: warning: function 'SW_RUN_deepCopy' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name] 35 | void SW_RUN_deepCopy( include/filefuncs.h:34:6: warning: function 'FileExists' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name] 34 | Bool FileExists(const char *f); include/generic.h:304:6: warning: function 'lobf' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name] 304 | void lobf(double *m, double *b, double xs[], double ys[], unsigned int size);
src/mymemory.c:109:16: warning: Potential leak of memory pointed to by 'p' [clang-analyzer-unix.Malloc] 109 | return NULL; // Exit function prematurely due to error src/mymemory.c:60:16: warning: Potential leak of memory pointed to by 'p' [clang-analyzer-unix.Malloc] 60 | return NULL; // Exit function prematurely due to error
src/SW_Output.c:1871:50: warning: Array access results in a null pointer dereference [clang-analyzer-core.NullDereference]
1871 | ForEachVegType(j) { s->SWA_VegType[j][i] = 0.; }
-> Setting `s->SWA_VegType` to 0 is handled by SW_SWC_alloc_outptrs()
-> The code actually never worked because (i) `s` has always been set to NULL and (ii) SW_OUT_construct() never returned `s` -- see commit 87e695b (Mar 6, 2018)
2 changes are confirmed with `tools/run_iwyu.sh` -- `iwyu` disagrees with other warnings
src/SW_Domain.c:8:1: warning: included header myMemory.h is not used directly [misc-include-cleaner]
8 | #include "include/myMemory.h" // for Str_Dup
src/SW_Main.c:22:1: warning: included header SW_Defines.h is not used directly [misc-include-cleaner]
22 | #include "include/SW_Defines.h" // for SW_OUTNKEYS
src/SW_Site.c:441:44: warning: repeated branch body in conditional chain [bugprone-branch-clone]
441 | if (ptf_type == sw_Cosby1984AndOthers) {
src/SW_Output.c:941:13: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
941 | case eSW_Estab: /* do nothing, no averaging required */
src/SW_Domain.c:221:9: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
221 | switch (keyID) {
src/SW_Output_outtext.c:201:5: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
201 | switch (pd) {
src/SW_Output_outtext.c:542:5: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
542 | switch (pd) {
src/SW_Site.c:1750:13: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
1750 | switch (k) {
src/SW_VegEstab.c:588:9: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
588 | switch (lineno) {
src/Times.c:171:41: warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator] 171 | return (TimeInt) ((yr > 100) ? yr : (yr < 50) ? 2000 + yr : 1900 + yr);
warning: do not use 'else' after 'return' [readability-else-after-return]
- identify accepted suppressions - check options
Error was introduced on 2024-July-17 with commit c9e99e3 "Fix bugprone-switch-missing-default-case"
warning: an assignment within an 'if' condition is bug-prone Also simplify the conditionals in `RemoveFiles()`
warning: performing an implicit widening conversion to type 'size_t' (aka 'unsigned long') of a multiplication performed in type 'LyrIndex' (aka 'unsigned int') warning: performing an implicit widening conversion to type 'unsigned long' of a multiplication performed in type 'int'
warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings
warning: multilevel pointer conversion from 'char **' to 'void *', please use explicit cast warning: multilevel pointer conversion from 'SW_VEGESTAB_INFO **' to 'void *', please use explicit cast warning: multilevel pointer conversion from 'SW_VEGESTAB_INFO **' to 'void *', please use explicit cast warning: multilevel pointer conversion from 'SW_WEATHER_HIST **' to 'void *', please use explicit cast warning: multilevel pointer conversion from 'char ***' to 'void *', please use explicit cast warning: multilevel pointer conversion from 'sw_converter_t **' (aka 'union cv_converter **') to 'void *', please use explicit cast
warning: declaration uses identifier '_begin_year', which is reserved in the global namespace warning: declaration uses identifier '_begin_day', which is reserved in the global namespace warning: declaration uses identifier '_end_day', which is reserved in the global namespace warning: declaration uses identifier '_ProjDir', which is a reserved identifier warning: declaration uses identifier '_notime', which is reserved in the global namespace warning: declaration uses identifier '_set_SXWrequests_helper', which is reserved in the global namespace warning: declaration uses identifier '_create_csv_headers', which is reserved in the global namespace warning: declaration uses identifier '_create_filename_ST', which is reserved in the global namespace warning: declaration uses identifier '_create_csv_file_ST', which is reserved in the global namespace warning: declaration uses identifier '_SWCMinVal', which is a reserved identifier warning: declaration uses identifier '_clear_hist', which is reserved in the global namespace warning: declaration uses identifier '_reset_swc', which is reserved in the global namespace warning: declaration uses identifier '_sanity_check', which is reserved in the global namespace warning: declaration uses identifier '_read_spp', which is reserved in the global namespace warning: declaration uses identifier '_checkit', which is reserved in the global namespace warning: declaration uses identifier '_zero_state', which is reserved in the global namespace warning: declaration uses identifier '_ProjDir', which is a reserved identifier (rename to 'SW_ProjDir')
warning: loop variable has narrower type 'IntUS' than iteration's upper bound 'int' warning: loop variable has narrower type 'IntUS' than iteration's upper bound 'LyrIndex' warning: loop variable has narrower type 'IntUS' than iteration's upper bound 'IntU'
warning: variable 'key2obj' is non-const and globally accessible, consider making it const warning: variable 'pd2str' is non-const and globally accessible, consider making it const warning: variable 'pd2longstr' is non-const and globally accessible, consider making it const warning: variable 'styp2str' is non-const and globally accessible, consider making it const warning: variable 'styp2longstr' is non-const and globally accessible, consider making it const warning: variable 'swrc2str' is non-const and globally accessible, consider making it const warning: variable 'ptf2str' is non-const and globally accessible, consider making it const warning: variable 'key2veg' is non-const and globally accessible, consider making it const warning: variable 'possKeys' is non-const and globally accessible, consider making it const warning: variable 'SWVarUnits' is non-const and globally accessible, consider making it const
warning: redundant explicit casting to the same type 'LyrIndex' (aka 'unsigned int') as the sub-expression, remove this casting warning: redundant explicit casting to the same type 'void (*)(OutPeriod, SW_RUN *, SW_OUT_DOM *)' (aka 'void (*)(unsigned short, struct SW_RUN *, struct SW_OUT_DOM *)') as the sub-expression, remove this casting warning: redundant explicit casting to the same type 'Bool' as the sub-expression, remove this casting warning: redundant explicit casting to the same type 'int' as the sub-expression, remove this casting Keep "warning: redundant explicit casting to the same type 'char *' as the sub-expression, remove this casting" due to test compilation which throws an warning
warning: 1st argument 'domID' (passed to 'varID') looks like it might be swapped with the 3rd, 'domDims' (passed to 'dimIDs')
This caused over 150 warning messages, the following warnings are a subset of them and make up most of the unique messages warning: 'sscanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' warning: 'atof' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtod' instead warning: 'atoi' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead Convert all `ato*()` functions to `strto*()` Program now uses macro `errno` for `strto*()` functions New function - `check_errno()` which checks errno after a call to `strto*()` and fails if a valid number was not produced
warning: the value returned by this function should not be disregarded; neglecting it may lead to errors
Update SOILWAT2 code base to pass clang-tidy checks - excluded checks, see ".clang-tidy" and ".clang-tidy_swtest" - requires clang-tidy v17 or later - make targets: "tidy-bin" and "tidy-test" - script: "tools/run_tidy.sh"
- Addresses issue #425 - Previously, when outputting to netCDFs, the bigger the domain, the longer it took to write output after every simulation run - Created a new output within `Input_nc/desc.in` - "deflateLevel" that specifies the level of deflation that will be sent into `nc_def_var_deflate()` * If the value is 0 (default), variables will not have any kind of deflation * Otherwise, the values 1-9 specify the intensity of deflation that will occur - New variable within SW_NETCDF to hold this new input - set to 0 in `SW_NC_init_ptrs()` - Added custom chunking to every non-dimensional variable (e.g., variables that are not time, vert, lat/lon, x/y, etc.) * Previously, the deflation would provide chunk sizes that encompass the entire domain - E.g., chunk size = [10, 10, 366] for a variable with xy = 10 x 10 sites and time = one leap year - The preferred chunk size for the example above should be chunk size = [1, 1, 366] - New function - `writeDummyVal` which is used to write a dummy value to a variable within the given netCDF and detects which type to write * Only use if `deflateLevel` is 0
- In the latest version of SOILWAT2 (v8.0.0), when the user creates a large domain, there is a noticeable increase in the runtime of individual simulations
- SOILWAT2 now makes use of
- Custom nc chunking
- A new user input - `deflateLevel`
- An initial write to all output nc files while creating them
- See #425 for more information
- Update to C++17 for attribute "maybe_unused" - close #427
- comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Werror=sign-compare]
Update GoogleTest Update tests to GoogleTest v1.15.2 Tests now require C++17
`make CPPFLAGS=-DSWNC` is equivalent to `make CPPFLAGS='-DSWNETCDF -DSWUDUNITS'` and `make CPPFLAGS=-DSWTXT` is equivalent to `make`
Feature flag SWNC Simplify compilation of nc-based SOILWAT2 to `make CPPFLAGS=-DSWNC`
- new target "clean_example" to delete output and input artifacts of the example simulation - targets that run the example simulation gain the "-r" option which automatically renames the domain template file in nc-mode
- now coefficient of the clay predictor is correct, see Table 4 of Cosby et al. 1984 - example SWRC ksat inputs are corrected - cf. commit 5653f7f "Added saturated hydraulic conductivity as parameter to swrcp" from June 10, 2022
- SWC-related variables need to be (re-)initialized after soils are updated
- tests for SWRCBulkSoilParameters() - water balance tests with soil organic matter > 0
…maters - for now, set om to 0 for water balance test
Close #397 The implemented approach has two steps: 1) Determine organic matter properties for the soil layers assuming fibric peat characteristics at the soil surface and characteristics of sapric peat at a user-specified depth (with a default of 50 cm). 2) Estimate bulk soil parameters of the soil water retention curve as linear combinations of properties for the mineral soil component and of properties for the organic matter soil component using the proportion of organic matter in the bulk soil as weights. New inputs * siteparam.in: depthSapric * soils.in: fractionWeight_om * swrc_params*.in: swrcpOM[fibric], swrcpOM[sapric] New functions * interpolateFibricSapric() to linearly interpolate organic matter parameter between fibric and sapric characteristics * SWRC_bulkSoilParameters() to calculate parameters of the bulk soil are as the weighted average of parameters of the organic and mineral components Functions with new arguments/behavior * SW_SIT_read() now reads in value for depthSapric from "siteparam.in" * SW_LYR_read() now reads in values for fractionWeight_om from "soils.in" * SW_SWRC_read() now reads in values for swrcpOM[fibric] and swrcpOM[sapric] from "swrc_params*.in" * SW_SIT_init_run() now runs checks for swrcpOM[fibric] and swrcpOM[sapric] and calculates bulk soil SWRCp from organic and mineral soil components via a call to SWRC_bulkSoilParameters() * SW_swcBulk_saturated() gained argument `"fom"` which is passed on to PTF_Saxton2006() if legacy `"Cosby1984AndOthers"` is selected as PTF * SW_swcBulk_minimum() and ui_theta_min() gained argument `"fom"` which is passed on to PTF_RawlsBrakensiek1985() if legacy `"Cosby1984AndOthers"` is selected as PTF * PTF_Saxton2006() gained argument `"fom"` and throws an error if fom > 0.08 (these published equations incorporate influence of organic matter up to 8% which was previously fixed at 0%) * PTF_RawlsBrakensiek1985() gained argument `"fom"` and throws an error if fom > 0 * SW_check_soil_properties() now checks that user-input `fractionWeight_om` is within 0-1 * set_soillayers() gained argument `"pom"` to set a value for `fractionWeight_om`
** This changes simulation output ** * Different outcomes during periods of high infiltration (e.g., snowmelt) and during conditions of low hydraulic conductivity (e.g., frozen soils, sapric organic matter). * Saturated percolation is now limited -- previously, it was unbounded. The upper bound is a function based on saturated hydraulic conductivity (which includes effects of organic matter), frozen soils, and a user-specified `"permeability"` factor. * Bulk soil saturated hydraulic conductivity parameter now accounts for flow pathways through organic matter above a threshold and assumes conductivities through mineral and organic components in series outside of those pathways.
- SWRC_bulkSoilParameters(): added documentation of argument "swrc_type" to fix documentation - SWRC_bulkSoilParameters(): corrected spelling of argument "swrcOM" to "swrcpOM" - tests "SWFlowSaturatedPercolation", "SWRCBulkSoilParameters" and "SitePTFRawlsBrakensiek1985": clang-tidy: use const variables where appropriate - test "SWFlowSaturatedPercolation": clang-format
- the variable standingWater was previously used uninitialized which lead to NaN when compiled with g++
Previously, SWRC_bulkSoilParameters() was called with "SW_Site->swrcpOM" as argument for "const double swrcpOM[2][SWRC_PARAM_NMAX]" to pass fibric and sapric organic matter SWRC parameters. Problems (here using gcc/g++ v14.2.0): - gcc warning (`CC=gcc make clean all`): "'SWRC_bulkSoilParameters' accessing 96 bytes in a region of size 48 [-Wstringop-overflow=]" - gcc error (`CC=gcc make clean bin_debug_severe`): "invalid use of pointers to arrays with different qualifiers in ISO C before C23 [-Werror=pedantic]" - g++ error (`CXX=g++ make clean test`): "cannot convert 'const double (*)[]' to 'const double (*)[6]'" Option 1: coerce call with "(const double (*)[SWRC_PARAM_NMAX]) SW_Site->swrcpOM" as argument for "const double swrcpOM[][SWRC_PARAM_NMAX]" - gcc warning (`CC=gcc make clean all`) resolved - gcc error (`CC=gcc make clean bin_debug_severe`) resolved - g++ error (`CXX=g++ make clean test`) resolved Option 2: remove "const", i.e., call with "SW_Site->swrcpOM" as argument for "double swrcpOM[][SWRC_PARAM_NMAX]" - gcc warning (`CC=gcc make clean all`) resolved - gcc error (`CC=gcc make clean bin_debug_severe`) resolved - g++ error (`CXX=g++ make clean test`) resolved This commit implements option 2. Thanks to @N1ckP3rsl3y
- commit 8029a9d introduced for the test SWFlowTest.SWFlowSaturatedPercolation a new variable "swc0init' to initialize soil water content - by mistake, the first use of "swc" was, however, not initialized -> this commit now initializes "swc[]" as intended
Feature soil organic content * SOILWAT2 now represent the influence of soil organic matter on the soil water retention curve and the saturated hydraulic conductivity parameter (issue #397) * Saturated percolation is now limited. The upper bound is a function based on the saturated hydraulic conductivity parameter (which includes effects of organic matter), frozen soils, and a user-specified `"permeability"` factor. * New input via `"siteparam.in"` to specify the depth at which characteristics of organic matter have completely switched from fibric to sapric peat * New input via `"soils.in"` to provide the proportion of soil organic matter to bulk soil by weight for each soil layer. * New input via `"swrc_params*.in"` to provide parameter values of the soil water retention curve representing fibric and sapric peat.
# Conflicts: # NEWS.md # include/SW_Weather.h # include/SW_datastructs.h # src/SW_Weather.c # tests/gtests/test_SW_Weather.cc # tests/gtests/test_WaterBalance.cc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is phase 1 of a multi-phase plan with the objectives of increasing our capabilities to carry out large simulation experiments, see milestone
Phase 0 (PR #378) included the introduction of a simulation domain (a set of simulation units/runs) and the capability to produce output as netCDFs. It resulted in in the release of SOILWAT2 v8.0.0.
Phase 1 includes the capability to acquire inputs from netCDFs and updates to vegetation and soils.
Later phases will add parallelization across the simulation domain.
Domain/netCDF
Vegetation
Soils
SOILWAT2 as library for STEPWAT2 and rSOILWAT2
Other improvements