Open edwardhartnett opened 2 years ago
fossell
commented
2 years ago @edwardhartnett - Thank you for providing this list. The DTC would like to help with the effort to initiate proper documentation in the existing code base as well as establish documentation standards for code developers. Would you be able/willing to meet to help get us started in the right direction or perhaps you have resources, examples already you can point us to?
edwardhartnett
commented
2 years ago I'm absolutely happy to help by meeting at any time.
Most of the warnings are pretty self-explanatory. Let's take an example:
/home/ed/UPP/sorc/ncep_post.fd/AVIATION.f:231: warning: Member calcat(U, V, H, U_OLD, V_OLD, H_OLD, CAT) (function) of file AVIATION.f is not documented.
So when I look at AVIATION.f, around line 231, here's what I see:
SUBROUTINE CALCAT(U,V,H,U_OLD,V_OLD,H_OLD,CAT)
!$$$ SUBPROGRAM DOCUMENTATION BLOCK
! . . .
! SUBPROGRAM: CALCAT COMPUTES Clear Air Turbulence Index
! PRGRMMR: Binbin Zhou /NCEP/EMC DATE: 2005-08-16
!
! ABSTRACT:
! This program computes the Clear Air Turbulence condition
! which is expressed as Index with Ellrod Algorithm
! (Gary P. Ellrod: Wea. and Forecast,1992) and Ri number
! suggested by S. Silberberg of AWC. But Ri number is still
! not classified into 3 level CAT, so current version does
! not use Ri as suggested by S. Silberberg
!
! PROGRAM HISTORY LOG:
!
! 05-09-19 H CHUANG - MODIFIED TO COMPUTE GRADIENTS FOR BOTH A AND E GRIDS
!
!
! According to Ellrod, the CAT is classied into 3 levels (index)
! Light: CAT = 1
! Middle: CAT = 2
! Severe: CAT = 3
! No CAT: CAT = 0
!
! USAGE: CALL CALCAT(U,V,H,L,CAT)
! INPUT ARGUMENT LIST:
! U - U wind profile (m/s) (at pressure level)
! V - V wind (m/s) (at pressure level)
! H - Height (m) (at pressure level)
! L - # of pressure level
!
! OUTPUT ARGUMENT LIST:
! CAT - CAT Index
!
! OUTPUT FILES:
! NONE
!
! SUBPROGRAMS CALLED:
! UTILITIES:
! LIBRARY:
! NONE
!
! ATTRIBUTES:
! LANGUAGE: FORTRAN 90/77
! MACHINE : BLUE AT NCEP
!$$$
use masks, only: dx, dy
So the problem here is that the legacy documentation has not been converted to doxygen. THe legacy documentation block should be removed, and a doxygen documentation block added, just before the SUBROUTINE line, which contains the documentation for this subroutine.
Once this documentation is converted to doxygen, this warning will go away.
fossell
commented
2 years ago @WenMeng-NOAA - Our team is starting to tackle this issue of translating current UPP routines to use Doxygen documentation syntax. As we make progress, would you prefer PRs for sets of files as they're ready, or wait until we make changes in all routines and submit the bulk of these documentation updates in one PR?
WenMeng-NOAA
commented
2 years ago @fossell Multiple PRs would be easy for reviewing and merging. Thanks!
edwardhartnett
commented
2 years ago I strongly recommend that each programmer working on doxygen conversion start with one code file, and put that up for PR, tagging me for review. As we work through the issues, each programmer will learn how to do documentation in a way that's similar to the way the rest of the UPP documentation is done.
Doxygen offers many alternatives - we don't want each programmer finding their own way of doing things - we want to follow common doxygen idioms, and have all our doxygen look similar. We have worked out a lot of the problems on the NCEPLIBS team, and the UPP team should not have to re-engineer all that.
Once each programmer gets one or two single-file doxygen PRs through, they can start doing 5 or 6 files per PR. No more than that please, if you want effective review. At that point I will bring in some other members of the NCEPLIBS team to help review the doxygen.
Also, please make all doxygen PRs separate from code changes. Doxygen PRs should have changes only to lines of comments - never to any lines of code. Keep your code changes for a separate PR after the documentation is done. This makes it easy to make large numbers of changes (as is required) without any chance of breaking code.
fossell
commented
2 years ago @edwardhartnett - thanks for your suggestions, this all sounds good to me. Appreciate your willingness to review these PRs and help us learn leverage from your experience. Our initial plan is to just convert the existing content to doxygen syntax, like you did with AVIATION.f, and not pursue any further enhancements at this time. Does that sound like a reasonable approach or do you recommend other things to consider?
edwardhartnett
commented
2 years ago That is a good approach and there is plenty of work there. In doing that, every programmer will learn the common doxygen tags, and such fancy tricks as tables, lists, and references.
Once that is complete, all the programmers involved will have a good understanding of doxygen. Those who are interested can read the documentation and tutorials to learn more advanced doxygen techniques and more ways to make the documentation beautiful as well as informative.
fossell
commented
2 years ago @WenMeng-NOAA - @kayeekayee is going to be ramping up work on these doxygen updates which means a handful of PRs coming through. We'd like to have @edwardhartnett review them, or at least an initial batch, to provide comments and suggestions before merging. Ka Yee had issues assigning Ed as a reviewer, but I'll work with her to resolve that problem. As future PRs for this come in we'll wait for Ed's review initially until we have a good feel for it. If you'd like us to take a different approach in coordinating these PRs with you, just let us know.
@edwardhartnett - Let us know if you're still willing to review a few of these for us. #451 has already been merged, but feel free to look or just comment on forthcoming PRs.
kayeekayee
commented
2 years ago @edwardhartnett @fossell Please review this PR if all the warnings are being fixed or not. Let me know if I missed any. Thank you.
edwardhartnett
commented
2 years ago Turn on WARN_AS_ERROR in your Doxyfile.in and doxygen will tell you. ;-)
fossell
commented
2 years ago @kayeekayee - It looks like the version of doxygen on Cheyenne is 1.8.6 and it's not recognizing the WARN_AS_ERROR tag, so it doesn't immediately fail when encountering an error. I tried on my laptop with v1.9.3 and it does fail as soon as there is an error. Even without this tag, on Cheyenne when I make the docs it produces a list of warnings about what is not completely documented yet. (I'm not sure about what version is on Hera and what the behavior is there)
In many cases it looks like it's certain parameters of routines you already started to translate to doxygen are not documented. I'm guessing that this is just a result of documentation that was never developers never put in there in the first place or updates to the code that were never documented, so during this initial phase of translating only what's existing, these are still missing complete documentation. For example, CALGUST.f produces a warning about LPBL, ZPBL, and GUST not being documented, because LPBL and ZPBL are not listed with an @param in the doxygen block, and GUST is listed as out, when in fact in the code it's inout.
Other routines may not have been updated at all, e.g. VRBLS2Dmod.f. In fact most module routines are not doxygenated. @edwardhartnett - Is there a flag/tag in the doxyfile.in that will list the files only (not all members of all files) that are not documented at all given a certain source directory? Without spitting out all of the warnings? The WARN* options I looked at didn't seem to quite get at that. Or is it best to just make run down the list of files in our source directory? Do all routines need to have a @something for doxygen to even consider it?
@kayeekayee - I think we have two things to attack here, 1) make sure all source code is being recognized and included by doxygen for the documentation, and 2) updating routines with incomplete documentation.
edwardhartnett
commented
2 years ago Doxygen will consider all files which are included in the Doxyfile INPUT field, and which have at the top of the file:
!> @file
There is not a lot of warning control. You get what you get. I think there is a separate warning setting for warnings for undocumented functions and warnings for missing parameters.
Indeed, different versions of doxygen respond slightly differently to warnings. On the NCEPLIBS project, we take 1.8.7 (IIRC) as canonical because that's what GitHub CI uses in its ubuntu-latest distribution.
It's expected that hand-maintained documentation will drift and contain mistakes, so it's not at all surprising that there are missing or incorrect parameters. You must simply work through the list and fix all the warnings. Then the WARN_AS_ERROR flag can be turned on, and you will never have to do it again. ;-)
Fortunately, fixing doxygen warnings is easy work.
fossell
commented
2 years ago @edwardhartnett - Thanks! This is very helpful. We will keep tackling this with the goal of being able to turn that WARN_AS_ERROR flag on and never look back. :)
fossell
commented
2 years ago @camshe - This is an ongoing effort so probably one to be aware of.
edwardhartnett
commented
3 weeks ago Can this issue be closed?
Doxygen gives warnings when things aren't quite right. I've added the current list of warnings, they can also be seen in the CI runs (after #390 is merged).
Once all these warnings are fixed, we can turn on the WARN_AS_ERROR flag, and that will cause the CI build to break if someone commits code without proper doxygen documentation. This is very worthwhile as it enforces some documentation standards automatically.