46 juergen documentation cable pop #61
Conversation
|
|
High-level documentation of the POP module. Some subroutines could not be documented due to insufficient understanding of the code and lack of knowledge wrt the purpose of the subroutines. Code contains many (apparently redundant, but to be checked) similar subroutines that do the same calculations on a different spatial organisational level (single patch vs. all patches). Presence of optional input parameters with unknown function. |
There was a problem hiding this comment.
Jurgen,
A couple of suggestions in line (one notable, the other typographic) - plus
- There are some existing comments that start with !************** These are being picked up by FORD as inline comments (at the MODULE level) and I think they don't need to be
- Have a think about whether (or not) it would be better to add in units (for the main inputs/outputs) for each SUBROUTINE.
As an aside we will have to break this MODULE up at some point.
…moved erroneous comments
src_pop/core/biogeochem/POP.F90
Outdated
|
|
||
| !! Patch Disturbance |
There was a problem hiding this comment.
did you really want these !! lines here? They appears as a rogue comment at the top pf the POP MODULE page - not in the Patch_disturb box
There was a problem hiding this comment.
These comments were indeed redundant and at the wrong place. I have removed them.
| ! POP receives StemNPP from CASA and returns woody turnover rate and vegetation height to the | ||
| ! main model. External inputs is disturbance regime (total or total and partial) and disturbance interval. | ||
| ! Main variables and their units are: | ||
| ! Inputs: |
There was a problem hiding this comment.
I'd include a blank line here to get this onto a new line
There was a problem hiding this comment.
I have added a few more blank lines. Looks like there is a tradeoff between readability of the code and readability of the rendered documentation.
There was a problem hiding this comment.
Hi Jurgen,
I'm generally happy with this - one comment about a rogue comment (before patch_disturb) and one suggestion.
More generally though the math typesetting needs looking at - it needs to be ( ... ) not $...$.
Perhaps also apply to the top level description as well (rather than as e.g. kg C m-2 yr-1)
|
@ccarouge I was going to approve (even merge in) this doc PR in but it looks like the process has changed and we're not deploying draft docs anymore. I wanted to check those before doing so - any advice? |
|
@har917 This is a very old PR. I know Juergen started this way and then went to do it differently (directly in the CABLE-POP_TRENDY branch I think). So I have on my todo list for a long time to review if this is still needed and same with the |
|
I think this one refers to the documentation of CABLE-POP only and not to code changes. I remember correctly, all that happened on a separate repository that's there specifically for documentation. It would be good if that one could be merged as it is almost complete and was just pending minor editorial suggestions. |
CABLE
Thank you for submitting a pull request to the CABLE Project.
Description
Please include a brief summary of the change and list the issues that are fixed.
Please also include relevant motivation and context.
You can link issues by using a supported keyword in the pull request's description or in a commit message:
Fixes #(issue)
Type of change
Please delete options that are not relevant.
Checklist
Please add a reviewer when ready for review.