Bugfix 117 connor #218
Merged
Bugfix 117 connor #218
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
Unit tests added for the following functions; svapor, petfunc, transp_weighted_avg, EsT_partitioning, pot_soil_evap, pot_soil_evap_bs, pot_transp, watrate, evap_fromSurface, remove_from_soil, infiltrate_water_low. infiltrate_water_low is lacking complete documentation.
Added unit tests for hydraulic_redistribution
Updated documentation and citations for SW_Flow_lib.c, SW_SoilWater.c, and SOILWAT2.bib
Added missing files to .bib file
|
Your latest commit is failing to compile on all CIs with error message: Please don't push code with compilation errors up onto github |
CaitlinA
requested changes
Jul 31, 2018
SW_Flow_lib.c
Outdated
|
|
||
| Equations based on Corbett and Crouse 1968. @cite Corbett1968 | ||
|
|
||
| @param *pptleft Remaining precipitation, (cm/day). |
There was a problem hiding this comment.
Fix tabs in the sections so they are aligned in the correct spot.
Also no comma is need before units i.e. Remaining precipitation (cm/day)
SW_Flow_lib.c
Outdated
| @brief Infiltrate water into soil layers under high water conditions, otherwise | ||
| known as saturated percolation. | ||
|
|
||
| @param swc Displays soilwater content in each layer before drainage, (cm^3 H2O). |
There was a problem hiding this comment.
Doublecheck: Do these superscripts come up as squared and cubed in the .html version?
SW_Flow_lib.c
Outdated
|
|
||
| Equations based on Penman 1948. @cite Penman1948 | ||
| Equations based on ASCE 2000. @cite ASCE2000 | ||
| Equations based on Bowen 1926. @cite Bowen1926 |
There was a problem hiding this comment.
This list of equations could be written as
Equations based on Penman 1948 @cite Penman 1948, ASCE 2000 .....,
SW_Flow_lib.c
Outdated
| @param cor[MAX_ST_RGR][MAX_LAYERS + 1] Two dimensional array containing soiltemperature data. | ||
| @param nlyrTemp The number of soil temperature layers. | ||
| @param depth_Temp Depths of soil temperature layers (cm). | ||
| @param sTempR Temperature values of soil temperature layers (C). |
There was a problem hiding this comment.
See if you can add a degree sign, or notation so that a degree sign appears on .html
SW_SoilWater.c
Outdated
| The equation and its coefficients are based on a | ||
| paper by Cosby,Hornberger,Clapp,Ginn, in WATER RESOURCES RESEARCH | ||
| June 1984. Moisture retention data was fit to the power function. | ||
| Equations are based on a paper by Cosby,Hornberger,Clapp,Ginn. @cite Cosby1984 |
There was a problem hiding this comment.
Keep note about moisture retention data being fit to the power function
test/test_SW_Flow_Lib.cc
Outdated
| EXPECT_DOUBLE_EQ(swc5[i], swcsat5[i]); // test that swc is now equal to or below swcsat in all layers but the top | ||
| swc5[i] = (int)(swc5[i] * 10000000 + 0.5)/ 10000000; // need to round for unit test, despite using EXPECT_DOUBLE_EQ | ||
| swcsat5[i] = (int)(swcsat5[i] * 10000000 + 0.5)/ 10000000; // need to round for unit test, despite using EXPECT_DOUBLE_EQ | ||
| EXPECT_DOUBLE_EQ(swc5[i], swcsat5[i]); // test that swc is now equal to or below swcsat in all layers but the top |
There was a problem hiding this comment.
Instead of rounding and then using EXPECT_DOUBLE_EQ what if we use EXPECT_NEAR instead. You test two numbers within a 'tolerance'. There is an example of its use in test_SW_SoilWater.cc
test/test_SW_Flow_Lib.cc
Outdated
| double expReturnTemp[] = {0.201, 0.245, 0.299, 0.364, 0.443, 0.539, 0.653, 0.788, | ||
| 0.948, 1.137, 0.136, 0.01, 0.032, 0.057, 0.060}; // These are the expected outcomes for the variable arads. | ||
|
|
||
|
|
There was a problem hiding this comment.
Get rid of empty space (one space/enter is typically fine for a break)
test/test_SW_Flow_Lib.cc
Outdated
| hydraulic_redistribution(swc1, swcwp1, lyrRootCo1, hydred1, nlyrs, maxCondroot, swp50, shapeCond, scale); | ||
|
|
||
| EXPECT_DOUBLE_EQ(hydred[0], 0); //no hydred in top layer | ||
| for(unsigned int i = 0; i < nlyrs; i++){ |
Updated documentaion for SW_Flow_lib.c and W_SoilWater.c, changed unit test for infiltrate_water_high function from EXPECT_DOUBLE_EQ to EXPECT_NEAR
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
=========================================
+ Coverage 64.94% 66.25% +1.3%
=========================================
Files 19 20 +1
Lines 3569 3621 +52
=========================================
+ Hits 2318 2399 +81
+ Misses 1251 1222 -29
Continue to review full report at Codecov.
|
Updated failing tests that were affected by if (st->lyrFrozen[i]) statements
CaitlinA
requested changes
Aug 6, 2018
test/test_SW_Flow_Lib.cc
Outdated
| EXPECT_DOUBLE_EQ(test, expReturnSlope[i]); //Tests the return of the petfunc against the expected return for the petfunc. | ||
|
|
||
| slope += 4.3; //Incrments slope input variable. | ||
| } |
There was a problem hiding this comment.
misaligned tab (just an example, fix through out)
Replaced all instances of EXPECT_DOUBLE_EQ combined with rounding with EXPECT_NEAR
|
Your PR is failing on some of your new tests. Please fix. |
Fixed failing tests dealing with st->lyrFrozen[i]
Fixed remaining failing tests at line 940.
|
@cjb643 You are still failing your unit-test specific failures - Please fix ASAP so we can wrap this up. |
Changes made to tests affected by if (st->lyrFrozen[i]) causing them to fail
Simplified a failing group of tests due to st->yrFrozen[i] receiving a load of value 1372188672, which is not a valid value for type 'Bool'
Changed EXPECT_LT(sTemp3[k], 100); to EXPECT_LT(sTemp3[k], 1000); at ln 569.
Added multiple resets. Undid change to line 569 of test_SW_Flow_lib_temp.cc
CaitlinA
previously approved these changes
Aug 21, 2018
- addressing #89 (Re-organize repository with more sub-folders)
…eptions) - fix #266 (comment) - addressing #266 - new script `tools/run_doxygen.sh` - new make targets: "doc" and "doc_clean" - continuous integration: travis-ci will now use the new script which fails on warnings, but disregards exceptions that are documented in the new file `doc/doxygen_exceptions.txt` - currently, there is one known exception, see #267 - moved `Doxyfile` to `doc/` (addressing #89)
This works well locally, but travis-ci fails with
```
# Examine log file for remaining warnings/errors
warnings=$(grep "warning\|error" ${log})
grep "warning\|error" ${log}
makefile:260: recipe for target 'doc' failed
make: *** [doc] Error 1
The command "make doc" exited with 2.
```
It is not clear to me why `grep "warning\|error" ${log}` would exit with code 2. As an attempt, I catch grep's stderr and assign it to `warnings` as well.
It is also not clear whether or not travis-ci expects that this script (called by `make doc`) should define an exit value if we got warnings (see https://docs.travis-ci.com/user/job-lifecycle#how-does-this-work-or-why-you-should-not-use-exit-in-build-steps).
- previous commit 6f6a058 didn't work - maybe travis-ci prefers extended (instead of basic) regular expressions? --> add `-E` option - for goodness sake, we now search for case insensitive warnings/errors -> option `-i`
- previous commit 6f6a058 didn't work - after installing and running travis locally, it seems that `grep` returns a non-zero exit status if nothing matches (i.e., we want nothing to match!) --> use grep ... || echo "" instead
- this commit uses a wrong filename for doxygen exceptions, i.e., the `make doc` should fail on travis-ci because it will find doxygen warnings - if indeed, travis-ci fails with this commit, then our continuous integration checks pass both positive and negative cases correctly
- commit 952e100 tested if travis-ci checks fail if `make doc` finds warnings: it did find warnings and printed them to the output; however, it did not fail --> add non-zero `exit` code to script if there are warnings
This reverts commit 952e100.
- link to latest doi release on zenodo - remove github downloads (since it wasn't doing what we wanted it for) - rename "Unix" badge to more appropriately "*nix"
- function `create_test_soillayers` can now use the fully fledged function `set_soillayers` (from commit "Merge branch 'feature_soillayerswap' into Bugfix_117_Connor") -- instead of the unit-test local and feature-incomplete `_set_layers` - small fix to README concerning installation of clang via macports
Merged
- include input files verbatim - this requires that the `doxyfile` "EXAMPLE_PATH" is set correctly
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
=========================================
+ Coverage 64.86% 66.3% +1.44%
=========================================
Files 19 20 +1
Lines 3569 3621 +52
=========================================
+ Hits 2315 2401 +86
+ Misses 1254 1220 -34
Continue to review full report at Codecov.
|
- clarified/expanded comments in input files for stand-alone use and for SOILWAT2 user manual - added notes for users to highlight dependencies and forcing inputs vs. parameters
- improve clarity of user manual - add output section
- general updates - added instructions on a minimal latex installation with tinytex
dschlaep
approved these changes
Oct 18, 2019
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.
closes #117 Documentation and citations have been added for all functions in SW_Flow_lib.c. Unit tests, have been added for the following functions in SW_Flow_lib.c;
infiltrate_water_high, infiltrate_water_low
petfunc
svapor
transp_weighted_avg
grass_EsT_partitioning, shrub_EsT_partitioning, tree_EsT_partitioning, forb_EsT_partitioning
pot_soil_evap
pot_soil_evap_bs
pot_transp
watrate
evap_litter_veg_surfaceWater
evap_fromSurface
remove_from_soil
hydraulic_redistribution
citations are found in SOILWAT2.bib.
Documentation was also added for all functions in SW_SoilWater.c #115