-
Notifications
You must be signed in to change notification settings - Fork 322
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
Small answer changes in preparation for adding option for bio-mass heat storage #1241
Merged
Merged
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b9b9c21
Preparation for biomass heat storage by rearrnaging terms a bit for d…
ekluzek 94f8223
Merge commit 'b9b9c216' into bhsprep_roffanschange
ekluzek 49b0e9f
Add checks that differences are within roundoff, get it working corre…
ekluzek eb7bdfe
Add new clm51 compsets with Sp mode, change one test to use it and ad…
ekluzek 501f5f8
Add some more terms to match the full BHS work, increase tolerance to…
ekluzek e3797fc
Do relative diffs rather than absolute, lower tolerance to 1.e-14, ra…
ekluzek 22fcf15
Remove temporary tests for terms being only different by relative dif…
ekluzek 5dfd628
Correct number of tests
ekluzek 5bb7e59
Work on ChangeLog
ekluzek f0abdd9
Update changelog
ekluzek 7410b45
Update change files
ekluzek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
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.
@ekluzek - Thanks for all of your work to demonstrate only roundoff-level diffs. I know from my own experience how painful that can sometimes be.
One question: are the terms you're checking here all order 1 (in magnitude)? If not, or if you're not sure, I generally use relative diff checks for things like this:
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 a good point. Actually I'm sure they aren't order 1. dt_veg and err will be order 1, but could also be very small. ulrad and dlrad should be order 100. So doing a relative diff would be a better comparison. If I do that I'm likely to be able to show a relative difference that's a bit smaller as well.
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.
No need to redo things, though, if you have already done it this way and you have a general sense of the magnitude. My main worry with abs diffs is if the terms are very small to begin with - so an abs diff of 1e-12 could be a relative diff of, say, 1e-6. If you're pretty confident that isn't generally the case here, then I think this is good enough.
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 went ahead and did a little testing with this to show it's fine. Relative diff's potentially a bit more troublesome if temp is identically zero, or negative. But with a relative diff I was able to show a tolerance of e-14 was fine for two tests.
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.
Just a thought for the future: If
temp
is the old (presumed correct) version, then I usually have success with something likeif (abs(temp - dt_veg(p)) > 1.e-13_r8 * temp)
– or change the lasttemp
toabs(temp)
if negative values are acceptable. This avoids divide by 0. If temp (the old version) was exactly 0 at a point, then I typically expect the new version to be exactly zero, too, and I generally want to ensure that's true (which is done by the above) – since exactly 0 vs. slightly different from 0 can sometimes lead to large downstream differences.