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.
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
User defined top snow layer #792
User defined top snow layer #792
Changes from 4 commits
63b2c81
20cbf74
3e5856a
986cf49
7f67efc
ee939a7
b2cf3a6
f293334
f49dac1
47cd874
ae415da
d31b2ee
ffe027c
ec4d91a
5bcf4d6
e00da7c
d7292f3
e478244
db4bb80
97cef78
bd7f647
4c19afc
1d5ea12
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In reference to this comment, the original code assigned huge(1._r8) to dzmax_l(12) and dzmax_u(12). So to match the original behavior, I think I need to do the following:
if (nlevsno >= 12) then
dzmax_l = huge(1._r8)
dzmax_u = huge(1._r8)
end if
Is this what you were suggesting @billsacks ?
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.
More precisely...
if (j >= 12) then
dzmax_l(j) = ...
dzmax_u(j) = ...
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.
Changing mind:
I prefer the first if statement with these corrections:
dzmax_l(nlevsno) = ...
dzmax_u(nlevsno) = ...
This replicates the current behavior for nlevsno = 12. If some day we allow nlevsno > 12, this just sets the bottom layer to huge.
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.
Yes, the last comment is what I was thinking (where the ... should be
huge(1._r8)
).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.e., the bottom layer should be set to huge, regardless of the number of snow layers. I now realize that that isn't exactly what the original code did, but I think that was the intent.
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 guess it would be good to verify that that does the right thing. Either (a) make that change and verify that it doesn't change answers for, say, a 5-layer case (a CLM45 case), or (b) don't make that change, and ensure that having dzmax_l and dzmax_u NOT set to huge for nlevsno gives the same answers as setting it to huge for a 12-layer case. I prefer (a), but don't feel strongly about it.
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 also prefer (a).
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.
The hobart test-suite includes a few clm45 cases and they all PASS.
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.
@billsacks I think the next four 0.01 values are what you wanted me to look for, right?
More generally speaking, I think this code is ready for review. Also I would like to know your preference in this case between new Unit Test and new standard test-suite test. I will be working on that next.
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.
Yes, these are the four places where I know this value is hard-coded. If you haven't already, thenI also thought it would be worth grepping the code for
0.01
to see if there are any other references that I'm not aware of.As for testing: With some upcoming suggestions that I'm going to make (in which you don't have hard-coded defaults for these values, but rather have them always explicitly set in the namelist), your new code will be sufficiently covered by existing system tests, so no need to add additional 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.
I looked for all the instances of 0.01 at the time, and there were some more that looked unrelated, so I left them alone.