New hamocc formatting

[mvertens]

This PR does the following:

• All hamocc code is now in modules
• There is new consistent indentation across all of the module files
• All implicit none statements are only at the module level
• Save statements at the module level have been removed
• There has been a cleanup of the history documentation blocks at the top of the many of the modules and subroutines

[mvertens]

@TomasTorsvik @jmaerz @JorgSchwinger - this PR is a substitution for the previous PR that had problems that @TomasTorsvik brought up. Hopefully, this is a more clean version. Although the PR comes from my fork - there is also a branch in the noresmhub repository that can be looked at.

[jmaerz]

@TomasTorsvik and @JorgSchwinger , since we briefly discussed the issue during lunch: it seems as if github isn't correctly showing the renaming. At least when I download Marianas branch and check it with gitk, the renaming also properly shows up (similar to when looking into the individual commits on github). Anyway, @mvertens : it is probably good to merge the full commit record (and not squash the commits).

[mvertens]

@jmaerz - I'm not sure what you mean by squashing the commit. This PR has implemented the steps that @TomasTorsvik suggested. First I simply renamed the code and committed that. Then I updated the code with the full changes that were part of the previous PR. Then I added lower case changes.

[jmaerz]

Hi @mvertens , once merging this PR into master, we should make sure that the full commit history is merged and not squashed.

[mvertens]

@jmaerz - thanks for your clarification. That makes total sense.

[jmaerz]

Hi @mvertens, @JorgSchwinger , @TomasTorsvik , I recognized that you removed numerous implicit none in subroutines. I am a bit uncertain, if we should still maintain the old style (incl. the implicit none in each subroutine, if possible, while I understand the intent of yours).

[mvertens]

@jmaerz - it is viewed as best practices now to only have implicit none at the module level. If you always use implicit none in your Fortran code, you only need to type it in programs and modules. If you want me to re-introduce it I will.

[mvertens]

Just to add that all new code in CESM has adopted this.

[mvertens]

@jmaerz - thank you so much for your careful review. I'll fix all of these tonight.

[jmaerz]

@jmaerz - it is viewed as best practices now to only have implicit none at the module level. If you always use implicit none in your Fortran code, you only need to type it in programs and modules. If you want me to re-introduce it I will.

Dear @mvertens , I was communicating about this with @TomasTorsvik , the only concern he raised was that when refactoring code, this information could get lost. But granted, with the now happened refactoring, this is probably ruled out. Any further opinion on this @JorgSchwinger @TomasTorsvik ?

[jmaerz]

Dear @mvertens , so many thanks for your great effort to make the code cleaner and modularize it. I am now once through the code. @TomasTorsvik and @JorgSchwinger , I cannot rule out that some things might have escaped the review, though. There are two things left (from my side). Minor: some files still contain (capital) THEN. The other part is: somehow, compiling on github fails, which needs to be solved before being able to merge.

[TomasTorsvik]

@jmaerz - it is viewed as best practices now to only have implicit none at the module level. If you always use implicit none in your Fortran code, you only need to type it in programs and modules. If you want me to re-introduce it I will.

I was thinking that it would be good to keep implicit none in case we refactor code and move subroutines around. But if we always encapsulate procedures within modules, I agree that we can keep implicit none at module level only.

[mvertens]

@jmaerz - thanks for your very careful review. I don't see any more instances of THEN. As long as I am still doing this - do you want me to: replace .GT. with > replace .LT. with < replace .EQ. with == replace .NE. with /=

It is trivial to do with across the code base with emacs. So as long as we are changing all the files - I'm happy to add this as well.

[jmaerz]

According from my trial with sed and the comments from @JorgSchwinger that I received, yes, exchanging .GT. etc. would be great.

[JorgSchwinger]

@jmaerz - it is viewed as best practices now to only have implicit none at the module level. If you always use implicit none in your Fortran code, you only need to type it in programs and modules. If you want me to re-introduce it I will.

I was thinking that it would be good to keep implicit none in case we refactor code and move subroutines around. But if we always encapsulate procedures within modules, I agree that we can keep implicit none at module level only.

I agree, too.

[jmaerz]

@mvertens wrt to the compiling issue, could you try to move subdir('hamocc') in ./meson.build (l.156) out of the if for if get_option('ecosys') to ca. l. 71 ? - that at least makes the stuff compilable on my local machine again, when ecosys is switched off. - I guess, it's then time to clean meson.build a bit... - with the new logicals.

[jmaerz]

Me again, sorry. The whole ecosys in ./meson.build can likely be reduced to:

subdir('hamocc')

if get_option('ecosys')
  add_project_arguments('-DHAMOCC', language: 'fortran')
endif

to switch on the one remained pre-processor flag (in BLOM to switch on HAMOCC).

[mvertens]

@jmaerz - I have made your suggested change to meson.build and pushed it back along with a lot of the other formatting changes.

[jmaerz]

getting an:

../hamocc/mo_write_netcdf_var.F90:209:5:

  209 |   end subroutine write_netcdf_var
      |     1
Error: Expecting END IF statement at (1)
../hamocc/mo_write_netcdf_var.F90:211:3:

  211 | end module mo_write_netcdf_var
      |   1
Error: Expecting END IF statement at (1)
f951: Error: Unexpected end of file in ‘../hamocc/mo_write_netcdf_var.F90’

on my side.

[mvertens]

@jmaerz - I pushed a fix back.

[mvertens]

The reason I did not catch this is because I'm compiling with -D_PNETCDF and you were not. This should work for you now.

[jmaerz]

Yeah, teamwork - running the stuff locally at the moment. Get some more issues... See also the github errors.

[jmaerz]

Line length issues again. The & are above the 132 limit in mo_ocprod - checking for more - once mo_ocprod is fixed, it runs on my side.

[mvertens]

Just fixed mo_ocprod.F90

[jmaerz]

Can you push it? - this should be all it needs to get the build system through.

[mvertens]

I've fixe two more files that are on the edge - and I'm verifying that it compiles right now. I'll push in a minute.

[jmaerz]

:-D - sorry, just curious at the moment ... - didn't mean to be pushy.

[mvertens]

@jmaerz - not push at all! I really appreciate your help!

[jmaerz]

Ok, runs through locally now :+1:

[mvertens]

Great. But github actions is still showing compilation errors.

[mvertens]

Maybe I spoke too soon?

[jmaerz]

Yes, I see this happen - it is likely related to the transfer of model parameters into mo_param_bgc and the namelist. For now, we could leave the parameters out of the namelist in mo_param_bgc - it is related to #294

[mvertens]

Should that be part of this PR?

[jmaerz]

It's a bit of a tricky task, since these values are also used and changed locally (and used in OMP directives) and we thought about fixing this in the beyond-CMIP6 branch after merging master... - but you're right, maybe we should do a separate attempt to fix this before merging.

[mvertens]

I'm happy to help with this. Can we talk about this now or in the morning? Whatever works better for you.

[jmaerz]

I feel it would be good to communicate with @JorgSchwinger and potentially @TomasTorsvik about it. It would mean to rename wpoc, wopal, wcalc, wdust to wxxxx_fix or wxxx_const and then add local variables in mo_ocprod that have the names wpoc, wopal, etc. - then rewriting the wpoc, etc. for the fix values in the sinking routine. I was previously hesitant to do this, since it touches the OMP directives with which I had formerly issues and slight answer changes - that whole part experiences fixes in beyond-CMIP6 anyway (since the OMP are not carefully written + there is a bug in the sinking routine - answer changing for the dust - and thus potentially/likely sediment).

So for now, the easier way might be to remove these variables from the namelist read in mo_param_bgc.

[mvertens]

@jmaerz - let me make sure I understand what you are saying.

• wcal in mo_ocprod.F90 is obtained via a use statement from mo_param_bgc.F90 (based on namelist input) but then is

  changed in statements like:
       if (use_AGG) then
              ..... 
              wcal   = wmass(i,j,k) <====
              ......

This is really confusing to me. Why would you change a namelist value you read in? So do we want to make wcal a local variable variable in mo_ocprod.F90 - say call it wcal_loc and use it there? And then have logic like the following

if (.not. use_AGG) then
    wcal_loc = wcal
endif
if (use_AGG) then
   .....
   wcal_loc = wmass(i,j,k)
   ...
else if (use_WLIN) then
   wcald = wcal_loc
else
   wcald = wcal_loc
endif

The private variable would be wcal_loc not wcal. Do you agree with this?

[jmaerz]

I sent you a zoom invitation

[jmaerz]

@TomasTorsvik and @JorgSchwinger , Mariana and I, we agreed on taking the sinking routine issue out to another PR - she'll set up a branch (we discussed on how to do it) - and then see, whether it breaks CMIP6 backwards compatibility or not. If not, it can go in to current master, otherwise, it will be a fix for the the beyond-CMIP6 branch as already laid out in #294.

[TomasTorsvik]

Thanks for catching the issue with wcal and friends. I agree that this is best addressed in a separate bug fix PR.

[jmaerz]

Dear @mvertens , I couldn't hold back from having a short other look into the routines this morning since I was for reviewing checking out a particular code version which you then changed afterward. Anyway, I just did a:

 grep -E '\b[[:upper:]]+\b' *F90

(not the most selective grep, I know, but provides an overview...) in the ./hamocc/ folder and there are a number of instances, where fortran key words still appear in capital fonts (END IF, MAX, SQRT, DATA, DIMENSION, ...). We can change this afterward, but if you want to have another look, go ahead.

[jmaerz]

Hi @mvertens , following up on your mo_ocprod fix comment in #306 , I would still suggest to do a separate PR for the mo_ocprod fix - while, since it seems to be bfb, it can enter master.

[mvertens]

I think all of the comments have been addressed for this PR.

[jmaerz]

Dear @mvertens , I couldn't hold back from having a short other look into the routines this morning since I was for reviewing checking out a particular code version which you then changed afterward. Anyway, I just did a:

 grep -E '\b[[:upper:]]+\b' *F90

(not the most selective grep, I know, but provides an overview...) in the ./hamocc/ folder and there are a number of instances, where fortran key words still appear in capital fonts (END IF, MAX, SQRT, DATA, DIMENSION, ...). We can change this afterward, but if you want to have another look, go ahead.

@mvertens Do you want to leave those for later? - just asking :-) - I am totally fine doing this later.

[mvertens]

@jmaerz - I've just pushed back the downcasing for MAX, MIN, DATA, SQRT and DIMENSION. Sorry for forgetting about those.

[jmaerz]

Dear @mvertens many thanks again! - I just want to confirm that the code also runs still locally and I would say, the changes can now be merged. Just as a gentle reminder - don't squash it to keep the renaming history. Thereafter #307 can go in (maybe it needs another update with master before - we'll see).