doxygen updates for code files involved in I/O and initialization#1174
doxygen updates for code files involved in I/O and initialization#1174edwardhartnett wants to merge 12 commits into
Conversation
| !> @param[in] ndse Error output unit number. | ||
| !> @param[in] ndst Test output unit number. | ||
| #ifdef W3_SMC | ||
| !> @param[in] mcel ??? |
There was a problem hiding this comment.
@MatthewMasarik-NOAA do you know what these parameters are? They are not documented in the source code.
There was a problem hiding this comment.
@ukmo-ccbunney would be a good POC for questions on SMC
There was a problem hiding this comment.
@ukmo-ccbunney, we were curious if you happen to know descriptions for these SMC grid parameters?
There was a problem hiding this comment.
@edwardhartnett - here are the descriptions for the SMC specific variables:
MCel - number of SMC cells
MUFc - number of SMC U-face connections
MVFc - number of SMC V-face connections
MRLv - number of SMC levels
MBSMC - number of SMC boundary points
MARC - number of SMC Arctic cells
MBAC - number of SMC Arctic boundary cells
MSPEC - Number of discrete spectral bins (not SMC specific)
|
@edwardhartnett - Thanks for this PR. I've tagged someone to hopefully help answer your question on SMC grid variables. In the meantime, I'm going to let @MatthewMasarik-NOAA take the lead on reviewing this PR since he's done the bulk of the doxygen work, so it'll be next week before we get this merged in. Please let me know if that causes any issues for you and we can re-assess if needed. |
|
I expected this to wait until Matt gets back. ;-) He and I have discussed this work, so it makes sense to wait. |
|
I think this should be merged, in spite of some missing documentation that is now '???'. There will be more cases of ??? in the documentation. We found it best on NCEPLIBS to go ahead and get the doxygen build to be checked for warnings in the CI build, and go back and fill in missing information later. This minor difficulty highlights the importance of checking the documentation in CI. When this code was submitted, @ukmo-ccbunney could no doubt have filled in the missing information in seconds. But since we did not collect that information with the code, we now have to chase it down, wasting team time and programmer time. Once we have fully doxygenated all the code, we can turn on warnings check. Once that is done, no programmer will be able to merge code that is not fully documented. That will save a lot of team time and result in excellent documentaiton with little effort. |
|
Hi all, |
ukmo-ccbunney
left a comment
There was a problem hiding this comment.
I've commented above w.r.t. the SMC variables.
| !> @param[in] ndse Error output unit number. | ||
| !> @param[in] ndst Test output unit number. | ||
| #ifdef W3_SMC | ||
| !> @param[in] mcel ??? |
There was a problem hiding this comment.
@edwardhartnett - here are the descriptions for the SMC specific variables:
MCel - number of SMC cells
MUFc - number of SMC U-face connections
MVFc - number of SMC V-face connections
MRLv - number of SMC levels
MBSMC - number of SMC boundary points
MARC - number of SMC Arctic cells
MBAC - number of SMC Arctic boundary cells
MSPEC - Number of discrete spectral bins (not SMC specific)
@edwardhartnett, I tagged @ukmo-ccbunney because is a good contact for SMC grid code. I wasn't suggesting he added that code or was responsible for documenting it. |
|
OK, updated... |
|
@edwardhartnett is this ready for review? |
|
Yes, ready for review and merge. |
MatthewMasarik-NOAA
left a comment
There was a problem hiding this comment.
@edwardhartnett I noticed that the legacy headers have been completely removed in these files. For the files I've done so far, since I would not be able to get all the details added in a "first pass", I've opted to leave the headers as-is until a first pass is complete. That way other people have a chance to see the state at that point, and decide if enough of the pertinent information had been converted to doxygen to feel comfortable removing the headers. Would that same approach work for you? I'm concerned about effectively giving you the the go-ahead to remove those headers wholesale when I had planned to save them through a first pass, when more of the user base could weigh in. @JessicaMeixner-NOAA, do you have any views on this?
|
I only removed legacy comments where I transferred all of the contents to Doxygen. Sometimes I corrected capitalization and punctuation, and of course the history logs I converted to markdown tables. But no information was removed. Everything that was there is now in the doxygen. |
|
From the files I reviewed that seems to be mostly the case, but not completely. For example, the header sections |
|
OK, I've added the error info back in, and the called by/calls is generated by doxygen and should not be maintained in documentation, since that is extra work, unneeded, and will undoubtably be wrong over time. Whereas doxygen will get the correct answer each time it is built... |
|
Hi @edwardhartnett, this is fine for this PR and I will proceed reviewing. I might not have conveyed my main concern well, and it's not that you doxygenate every single small thing in the current headers, it's that we aren't completely removing the original headers that have been a part of the code base up until now, without more notice to the other stakeholders. This is less about this PR, and more about an overall strategy if we are talking about doxygnating more of the code base. For this we have already set forward on a path, and it has been to do a first pass, which gets most of the required information doxygenated. The headers are left intact, assuming they will eventually be ripped out, but not until we finish the first step. Before moving forward with any doxygenating efforts for other files not related to this point output work, we need to have a discussion on strategy including @JessicaMeixner-NOAA. |
|
OK, I was going by this in #551
If you wish to keep old comments we can do so, though this adds a lot of code clutter, IMO, and programmers will be wondering which to update, or if they have to update both. However, you decide and let me know. We can leave them or take them out, after moving all the info. But if we leave them, there will be no way to distingish which you have done a first pass for, and which have had all information completely extracted. |
|
@edwardhartnett I support @MatthewMasarik-NOAA decision in how he wants to proceed and will follow his lead on this. |
|
I have not heard any request to start doxygenating the rest of the code base at the moment. It is definitely a goal, but my understanding, @JessicaMeixner-NOAA please correct me if I'm wrong, is we have more than one priority that is fairly urgent, and certainly ahead of completing the doxygen documentation. |
|
Absolutely. I will rework and circle around again with this later. |
Pull Request Summary
This PR contains doxygen improvements in three code files. No code changes are made, just documentation.
Description
As discussed with @MatthewMasarik-NOAA these files have been updated to doxygen.
Matt, review carefully and comment on anything you don't like.
Another round of edits will be necessary to document all the module variables, but I will do that as a separate PR, for ease of reviewing.
Issue(s) addressed
Part of #586
Part of #748
Commit Message
Doxygen updates
Check list
Testing
No code changes in this PR, just documentation.