-
Notifications
You must be signed in to change notification settings - Fork 6
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
Range checks cable parameters #287
Conversation
@abhaasgoyal I'm not sure what happened but for some reason, some comments got posted before I finished the review.. I'm still working on it but I'm getting to the end. |
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.
I was trying to finish this review but I have a relatively important question about cable_checks.F90
that I think we need to resolve before I continue with the review.
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.
I think I agree with the design of the changes in the rest of the code but I have a few comments.
We'll also need to look at the documentation.
After reviewing this, I think we should have split this work into 3:
But I guess it's easier to see with the final version than at the start. I'm just mentioning it as something to try and learn from for later work. |
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.
That's great. I think we getting there. I just had an idea with the elemental functions which I think is worth putting in. The rest is trivial.
Note: Before merging, I'll squash the commits into one with message and appropriate description |
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.
All good!! Well done.
@abhaasgoyal actually don't merge! I'm missing the info on what tests have been run after review. |
fcbab92
to
0e8df87
Compare
Tested using custom-build in
The output comparisons differ in defining headers for # Tail outputs file names along with the contents
tail -n +1 * | sed -n '/Failed to find variable RootResp/!p' | sed -n '/Failed to find variable StemResp/!p' | tee temp There seem to be no differences in some tests (like The errors seem to be showing the following template:
Update: When run with
The variables are out of range in the range of -10%-70%. The best I could suspect is that in |
Tested with the following configurations:
Then Status As of now, the bitwise comparisons of the output files pass in all configs. Output (provided only for case 1, case 2 and 3 were the same
|
@abhaasgoyal Good to go! 🎉 |
Fixes #225 Add 2 flags to range checks ``` check%ranges (0, 1, 2) - When to do the range checks NO_CHECK, ON_TIMESTEP, ON_WRITE check%exit (.true., .false.) - Whether to exit the program if range checks are done, and out of range - the other option is to provide a warning ``` - Checks are being done in the following stages 1. Parameters of `soil` and `veg` before running the spinloop 2. Parameters when creating the NetCDF output file (some of them clash with `soil` and `veg` but code can be refactored in the future to account for that) 3. Landuse variables I/O 4. Variables in creating restart file 5. Variables at every timestep - Redesign `cable_output` with introducing `generate_out_write_acc`, and `check_and_write` - Change the range checks to be done in `cable_write` during `cable_output`. - Make `output_inclusion_type` dependencies clear between group output and individual values Tested with experiments in `bench_example` for `main` vs current commit. <!-- readthedocs-preview cable start --> ---- 📚 Documentation preview 📚: https://cable--287.org.readthedocs.build/en/287/ <!-- readthedocs-preview cable end -->
8552247
to
3084239
Compare
CABLE
Description
Fixes #225
Add 2 flags to range checks
soil
andveg
before running the spinloopsoil
andveg
but code can be refactored in the future to account for that)cable_output
with introducinggenerate_out_write_acc
, andcheck_and_write
cable_write
duringcable_output
.output_inclusion_type
dependencies clear between group output and individual valuesTested with experiments in
bench_example
formain
vs current commit.📚 Documentation preview 📚: https://cable--287.org.readthedocs.build/en/287/