-
Notifications
You must be signed in to change notification settings - Fork 4
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
Cleaned up synth tests and fixed related bugs #23
Conversation
…rus simulations against which to compare LGAR-C results, so now there are two synth cases in total. Also fixed a bug that happened during the time step for which the top WF saturated, exposed during synth testing.
…ntegrated testing
…ts dzdt value was 0, not because it was a to_bottom WF but because dzdt numerically evaluated to 0 because theta was very close to theta_r
This reverts commit d311d82. 'reverting because integrated test failed'
…d test did not make the integrated tests pass
if ( (theta>=soil_properties[soil_num].theta_e) && (psi_cm_loc!=0.0) ){ | ||
//addresses a very rare case. Sometimes when psi gets very close to 0 but is not 0, calc_theta_from_h will actually yield theta_e for a very small nonzero psi value (for example psi=1e-3 or something like that). | ||
//This can happen for example when the model domain is very close to saturation, and the number of WFs == the number of layers, but there is a little bit of AET so the resulting model state should have just slightly less water than complete saturation. | ||
//However, layers above the current one might not have the property that this small zonzero psi value yields theta = theta_e. | ||
//This leads to the case where the mass balance correctly closes, but with theta=theta_e for the current layer and not with theta = theta_e for some higher layer(s). | ||
//Then, later, the psi value for the current layer is set to 0.0 because its theta value is theta_e, and then the layers above will have their psi values set to 0.0, and then theta = theta_e everywhere in the model domain, whereas it should account for the recent small AET that happened. | ||
//Just setting the AET to 0 in these cases closes mass balance; another option would be to augment the recharge. Error is very rare and seems to happen once every 100k parameter sets or so, using yearlong simulations. | ||
*AET_demand_cm = 0.0; | ||
} |
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.
this is a bit hard for me to follow, for any layer if psi is so small, theta is almost theta_e and we should enforce this for all layers, I would say
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.
Everything looks good. Thanks Peter
Updated synthetic tests, related visualizations, and a few bugs related to the synth tests / discovered in stability testing.
Additions
Removals
-At some point, we removed python and HYDRUS outputs for a synthetic test. So outputs in tests related to synthetic3 have been removed.
-Removed option 0 as a synth test that is indicated in the readme in tests. I left in the capability to run synth_0 however because it is used in integrated testing.
Changes
-Updated synthetic tests 1 and 2 to have accurate outputs: involved replacing data_layers, data_variables, and output comparison plots for synthetic tests 1 and 2.
-Updated jupyter notebook that generates output plots, now has dynamic paths.
-Fixed a bug that happened during the time step for which the top WF saturated, exposed during synth testing.
-Fixed a bug where the lowest WF would go below the lower boundary of the model domain.
-For some parameter sets, a very small but nonzero capillary head value (say 1e-3 cm or so) will mathematically yield a theta equal to theta_e, whereas mass balance in this case could require that theta is very slightly less than theta_e. Resolved a few bugs related to this concept, discovered from broad testing over 40k parameter sets.
-In a similar way (similar in the sense that it has to do with possible psi values and extreme sensitivity in the psi-theta relationship), some parameter sets will mathematically yield a dzdt of 0 when psi is very large, whereas these cases should have a very small but nonzero dzdt. Fixed this bug.
-Added an insanity check for dzdt. While a very small delta theta value can create a dzdt that is enormous and this is mathematically correct, very large dzdt values are not physically meaningful.
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support
Accessibility
Other