Add more documentation to history tape code#2020
Conversation
* Add more breadcrumbs between related variables and methods * Put related namelist flags/methods together * Update some out of date comments Doc-only change.
ekluzek
left a comment
ekluzek
left a comment
There was a problem hiding this comment.
This is some great improvements to the documentation, and naming of some variables as well as the ordering of some things. There's a few suggestions I have about changes, but largely this is good to go. There's not a lot of code that actually changes here, so this all seems quite safe.
| logical, public :: & | ||
| hist_empty_htapes = .false. ! namelist: disable default-active history fields (which | ||
| ! only exist on history tape 1). Overridden by hist_fincl1 | ||
| ! flag. |
There was a problem hiding this comment.
Hmmm, my first reaction here is that it isn't "overridden" by the hist_fincl1 flag. But, in a way it is in that anything in hist_fincl1 -- will be output. The way I've thought about it is that this turns all the default fields off -- and you have to explicitly add the list that you want to hist_fincl1.
There was a problem hiding this comment.
Makes sense, I reworded to "Use hist_fincl1 to enable select fields on top of this."
|
|
||
| character(len=max_namlen+2), public :: & | ||
| hist_fincl1(max_flds) = ' ' ! namelist: list of fields to add | ||
| hist_fincl1(max_flds) = ' ' ! namelist: list of fields to include in history tape 1 |
There was a problem hiding this comment.
I wonder if you should also include something that says "the "h0" set of files" or something to that effect?
There was a problem hiding this comment.
This sounds like documentation on the mapping between history tapes and filenames. I've added a note about this to the history_tape type and the set_hist_filename method; and put a breadcrumb to history_tape at the very top of the file.
There was a problem hiding this comment.
Yes, exactly, we call it "history tape 1", but the filenames are actually labeled as "h0". This is kind of an awkward thing we've been preserving. I like what you added below for history_tape and the breadcrumbs you put for pointing to it. Maybe there could be another one on top of the hist_fincl and hist_fexcl definitions?
There was a problem hiding this comment.
SGTM. How do I add a change at this point - looks like this commit got pulled into PR #2031, so should I just send a new PR for this final thing?
There was a problem hiding this comment.
You could either: create a new PR, add it to #2022 which is still outstanding, or add it to here, and this PR will stay open and I can pull it into another tag. I have a slight preference for adding to #2022, but any of these would work fine. So I don't care too much.
Because we run a bunch of testing by hand to create our tags, I am very likely to merge a few PR's together to create the tag (especially if they are simple bit-for-bit type things). So you should expect that. But, it's also easier to see things broken up into smallish PR's so you can see what's going on with a specific PR. So I do still like having things broken up into separate PR's.
There was a problem hiding this comment.
Added PR #2034 (note its commit is on top of the commits in this PR)
ekluzek
added
code health
2 participants
Description of changes
Add more documentation to history tape code
Doc-only change.
Specific notes
Contributors other than yourself, if any: none
CTSM Issues Fixed (include github issue #): none fixed, but preparation for fixing issue 27.
Are answers expected to change (and if so in what way)? no
Any User Interface Changes (namelist or namelist defaults changes)? no
Testing performed, if any:
Ran unit tests to confirm new comments didn't break compilation