Skip to content
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

doxygenated fv3_cap.F90 #819

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

AlysonStahl-NOAA
Copy link
Collaborator

@AlysonStahl-NOAA AlysonStahl-NOAA commented Apr 10, 2024

Description

More doxygen

Issue(s) addressed

Part of #760

Testing

Documentation changes only

Dependencies

N/A

Copy link
Collaborator

@edwardhartnett edwardhartnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment changes only in this PR, no code changes.

@junwang-noaa or @DusanJovic-NOAA can you help by telling us what the "???" should be?

module fv_moving_nest_main_mod

#include <fms_platform.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there so many code changes in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to investigate why it's showing up this way. I double checked this file in your repo and compared it to develop. It showed zero code changes, but after swapping the files for some reason it's showing up as huge chunks of changes. I followed the same procedure as the other 2 files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what happened, but it's fixed now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks!

Comment on lines +63 to +75
!> ???
type(ESMF_GridComp) :: fcstComp

!> ???
type(ESMF_State) :: fcstState

!> ???
type(ESMF_FieldBundle), allocatable :: fcstFB(:)

!> ???
integer,dimension(:), allocatable :: fcstPetList

!> ???
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these variables are local (private) variables and are not accessible outside the module. Are you going to add doxygen the every local variable in every module and subroutine? I thought we are adding doxygen just to subroutine arguments and public module variables.

Copy link
Collaborator

@edwardhartnett edwardhartnett Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not be adding doxygen for any subprogram variables.

Good question about private module variables, I will investigate and get back to you. My hunch is that doxygen doesn't distinguish between different types of module variables.

We need to document all the module variables that doxygen will complain about if undocumented. This is so we can turn on warning-check and ensure that no undocumented code is committed to the repo.

@edwardhartnett
Copy link
Collaborator

@AlexanderRichert-NOAA can you get the fv3atm CI working again?

@AlexanderRichert-NOAA
Copy link
Contributor

@DusanJovic-NOAA what's the status of the mpi_f08 issue, did you work out a fix?

@DusanJovic-NOAA
Copy link
Collaborator

@AlexanderRichert-NOAA Yes, mpi_f08 will be fixed in #805, which is merged with #811 which will be merged soon. But unfortunately there was an update in upp which is causing workflow to fail now at:

sed -i 's/doc /upp_doc /' upp/docs/CMakeLists.txt

So, we'll need one more update. Do you want to fix this (remove that sed command) in your 'Add unit testing' PR?

@edwardhartnett
Copy link
Collaborator

It's not a rush, if a fix is coming we can wait for it...

I just wanted to get the CI working again so we can start running doxygen in it.

@AlexanderRichert-NOAA
Copy link
Contributor

I would recommend fixing the sed failure in #811, just so we don't keep merging PRs where the CI isn't working. That said, I agree it should be as simple as removing that line since the custom target in UPP now has the name 'upp_doc' already.

@DusanJovic-NOAA
Copy link
Collaborator

ufs-wm code managers already started or actually finished regression testing, so that change must go in as part of the next PR.

@DusanJovic-NOAA
Copy link
Collaborator

@AlexanderRichert-NOAA @edwardhartnett I removed that sed command from fv3atm workflow file that renames 'doc' to 'upp_doc' in the UPP's CMakeLists.txt but the workflow is still failing:

https://github.com/NOAA-EMC/fv3atm/actions/runs/8833224366/job/24252100176#step:6:220

There is a mismatch in the name of doc folders in UPP version fv3atm is using. I see that is fixed 3 days ago in UPP https://github.com/NOAA-EMC/UPP/pull/932/files

@WenMeng-NOAA Looks like we will need another fv3atm PR that updates UPP submodule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants