Some cosmetic changes #299

JorgSchwinger · 2023-11-03T10:02:14Z

I went through the code and prettied a few things up a bit (as I believe). If there is something controversial I'm happy to revert it.

jmaerz · 2023-11-03T10:23:26Z

hamocc/ncout_hamocc.F90

  if (use_cisonew) then
     call ncdefvar3d(SRF_ATMC13(iogrp),cmpflg,'p',                                 &
          &   'atmc13','Atmospheric 13CO2',' ','ppm',0)
     call ncdefvar3d(SRF_ATMC14(iogrp),cmpflg,'p',                                 &
          &   'atmc14','Atmospheric 14CO2',' ','ppm',0)
-  end if
+  endif

  ! --- define 3d layer fields
  call ncdefvar3d(LYR_DP(iogrp),cmpflg,'p',                                     &


missed this & sign

jmaerz · 2023-11-03T10:24:13Z

hamocc/ncout_hamocc.F90

  if (use_BROMO) then
     call ncdefvar3d(LYR_BROMO(iogrp),cmpflg,'p',                                  &
          &   'bromo','Bromoform',' ','mol CHBr3 m-3',1)
-  end if
+  endif

  ! --- define 3d level fields
  call ncdefvar3d(LVL_DIC(iogrp),cmpflg,'p',                                    &


and these ones

jmaerz · 2023-11-03T10:31:31Z

hamocc/mo_sedmnt.F90

-      !   - solid volume fraction at cell center
-      !   - vertical molecular diffusion coefficients scaled with porosity
-      !
+!


Admittedly, I am not really happy with loosing the indentation here for the comments, since they belong to what comes after, which is also indented. But that might be just my personal matter and if you prefer it like this, I'll adapt.

@jmaerz - is your preference to have all comments not indented? Is that the general consensus for hamocc?

No, just the opposite. I would want to have the comments aligned to the code - I was just about to suggest this as a general comment - but it is something to generally agree on.

We totally agree. Best free-format practices recommend - 'Indent blocks by 2 characters. Where possible, comments should be indented with the code within a block'.

I also generally agree - in this specific case, however, I was interpreting the comment as a header of the subroutine and therefore aligned it with the SUBROUTINE statement above

Right, if it would have been written above the subroutine (which I wouldn't want) I would agree. Admittedly, here and also in other places, I would prefer indentation (there are a few more places, where this holds). But again, I am also fine, if you prefer it differently.

jmaerz · 2023-11-03T10:42:17Z

Hi @JorgSchwinger , ok, seems as if now, I am becoming a bit picky, sorry for that, but: could we think about aligning comments in general with the respective code? - e.g. in carchm , I find it very confusing, where the comment belongs to. etc. - with indenting comments, it becomes clearer, to which code they belong. Further, they visually don't break indentations of code (BTW: this is similarly true and could be done for OMP, while I there understand the reasoning for non-indentation a bit better).

jmaerz · 2023-11-03T10:49:22Z

Another thing would be the small or capital writing of fortran command, but maybe this goes beyond what we should care about right now. For those, I would prefer to keep MODULE CONTAINS END MODULE in capital, while the rest would be fine also in small. Anyway, this probably is too much for now.

JorgSchwinger · 2023-11-03T10:52:50Z

hamocc/mo_intfcblom.F90

+                             + wts2*powtra(i,j,k,:)
+           burial2(i,j,m,:)  = wts1*burial2(i,j,m,:)    &
+                             + wts2*burial2(i,j,n,:)    &
+                             + wts2*burial(i,j,:)


@mvertens
this is a good example of what I mean with the vertical alignment. I guess these changes would be reverted by an auto-indent?

@JorgSchwinger - yes it is. Thanks for the clarification. I agree that these changes would be reverted.
Not sure if we could automate that. Maybe a meeting to discuss all of these issues would be good.

jmaerz · 2023-11-03T10:55:07Z

Looks like we opened a can of worms :-)

jmaerz · 2023-11-03T11:03:23Z

Mmh, since this PR provokes some comments, I would suggest to collect them afterward in a small document (or updating older documents on best practices for iHAMOCC coding style).

jmaerz · 2023-11-03T11:33:52Z

hamocc/aufr_bgc.F90

        ncstat = NF90_OPEN(rstfnm,NF90_NOWRITE, ncid)
        IF ( ncstat .NE. NF90_NOERR ) THEN
-             CALL xchalt('(AUFR: Problem with netCDF1)')
-                    stop '(AUFR: Problem with netCDF1)'
+           CALL xchalt('(AUFR: Problem with netCDF1)')


Admittedly, the indentation of the stop is unclear to me (if it is about aligning the message, you could just shift the message). Go as you like.

Agree, will fix this.

JorgSchwinger · 2023-11-03T11:45:32Z

I agree, this seems to be a can of worms...

It looks like we will have to discuss things a bit first. I would suggest that we sit together and update the HAMOCC coding guidelines that we already made some time ago (should be also put into the repository).

For now, I would suggest to leave it here (I have one more pending request though) such that we can proceed making the release v1.4.0 and start working towards v1.5.0.

jmaerz · 2023-11-03T11:47:38Z

hamocc/ocprod.F90

@@ -100,7 +101,6 @@ subroutine ocprod(kpie,kpje,kpke,kbnd,pdlxp,pdlyp,pddpo,omask,ptho,pi_ph)
                            use_BROMO,use_AGG,use_PBGC_OCNP_TIMESTEP,use_FB_BGC_OCE,use_AGG,use_cisonew,use_natDIC,                &
                            use_WLIN,use_sedbypass
  use mo_vgrid,       only: dp_min,dp_min_sink,k0100,k0500,k1000,k2000,k4000,kwrbioz,ptiestu
-  use mod_xc,         only: mnproc
  use mo_biomod,      only: eps3d,bifr13_perm,growth_co2


mo_biomod could go up (to the first mo_biomod)

jmaerz · 2023-11-03T11:48:50Z

hamocc/ocprod.F90

+                                        + ocetra(i,j,kdonor,icalc13)*wcald)/       &
+                                         (pddpo(i,j,k)+wcal)
+                 ocetra(i,j,k,icalc14) = (ocetra(i,j,k     ,icalc14)*pddpo(i,j,k)  &
+                      	                 + ocetra(i,j,kdonor,icalc14)*wcald)/       &


& sign indent

jmaerz

Hi @JorgSchwinger , many thanks for the effort to bring the code to a better shape! - I am fine to approve (and do so), but I would suggest that we note down some (further) suggestions that we have, when browsing your changes. We had this already on the bucket list since a while and now the PR gives rise to further thoughts on it. From my side, I came up with a few general thoughts when browsing the changes:

How much indent per code block (I would also opt for 2-3)
Some routines/modules still have the +7 F77 indent, which could be changed to a smaller indent
I agree with your 'indent by meaning approach' when aligning following up lines, which likely would mean no automated indention via emacs.
Wrt spatial loops: any way is fine, I would say.
I would be in favor of aligning comments to code blocks (to not break the visual code flow) - similarly, one could think about OMP directives
small versus capital writing of Fortran commands (as written before, I would prefer capital for module, contains, implicit none, potentially subroutines, the rest I would be fine either way - like integer, float, if else endif constructs)
Indention of comments on subroutines?

Ok, I guess, this is the collection of mine for now, but others might have more - I would compile this list also in the discussion #298, where we can follow up on it.

mvertens · 2023-11-03T12:24:17Z

@jmaerz - thanks so much for compiling this list. That's super helpful and I agree should be moved to a discussion. I think we need to consider if we want a tool to go through the code and format it as a first pass and then fix things we don't like. It does not have to be emacs. Another point to consider is that most auto-formatters will not handle f77 code - but deal with free format code. At any rate this is a good discussion to have and come to a consensus on.

TomasTorsvik

@JorgSchwinger - Thanks! This look fine to me 👍

TomasTorsvik · 2023-11-03T12:05:55Z

hamocc/aufw_bgc.F90

@@ -178,7 +177,7 @@ SUBROUTINE AUFW_BGC(kpie,kpje,kpke,ntr,ntrbgc,itrbgc,trc,              &
         ncstat = NF90_CREATE(rstfnm,NF90_64BIT_OFFSET,ncid)
         IF ( ncstat .NE. NF90_NOERR ) THEN
            call xchalt('(AUFW: Problem with netCDF1)')
-            stop '(AUFW: Problem with netCDF1)'
+                   stop '(AUFW: Problem with netCDF1)'


Same indentation as for aufr_bgc. Just a reminder to fix it in both files.

TomasTorsvik · 2023-11-03T12:30:47Z

@JorgSchwinger @jmaerz
Once this is merged, are we ready for a release-1.4 (i.e. still CMIP6 compatible) from iHAMOCC side?

jmaerz · 2023-11-03T12:33:26Z

@TomasTorsvik , I think, #300 should also come in, right? - I'll update #289

JorgSchwinger requested review from TomasTorsvik and jmaerz November 3, 2023 10:02

jmaerz reviewed Nov 3, 2023

View reviewed changes

JorgSchwinger commented Nov 3, 2023

View reviewed changes

jmaerz reviewed Nov 3, 2023

View reviewed changes

jmaerz approved these changes Nov 3, 2023

View reviewed changes

TomasTorsvik approved these changes Nov 3, 2023

View reviewed changes

jmaerz mentioned this pull request Nov 3, 2023

Merging strategy for new release versions (CMIP6-compatible and INES interim release) #289

Closed

jmaerz added the iHAMOCC Issue mainly concerns the iHAMOCC code base label Nov 3, 2023

jmaerz added this to the preparing for noresm2.1 milestone Nov 3, 2023

JorgSchwinger added 3 commits November 3, 2023 15:20

Some cosmetic code changes

39ddb68

Small corrections

2d82d71

Fix issues raised in PR review

3513770

JorgSchwinger merged commit ca79c69 into NorESMhub:master Nov 3, 2023

JorgSchwinger deleted the feature-some_cosmetic_changes branch November 3, 2023 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some cosmetic changes #299

Some cosmetic changes #299

JorgSchwinger commented Nov 3, 2023

jmaerz Nov 3, 2023

jmaerz Nov 3, 2023

jmaerz Nov 3, 2023

mvertens Nov 3, 2023

jmaerz Nov 3, 2023

mvertens Nov 3, 2023

JorgSchwinger Nov 3, 2023

jmaerz Nov 3, 2023

jmaerz commented Nov 3, 2023

jmaerz commented Nov 3, 2023

JorgSchwinger Nov 3, 2023

mvertens Nov 3, 2023

jmaerz commented Nov 3, 2023

jmaerz commented Nov 3, 2023

jmaerz Nov 3, 2023

JorgSchwinger Nov 3, 2023

JorgSchwinger commented Nov 3, 2023

jmaerz Nov 3, 2023

jmaerz Nov 3, 2023

jmaerz left a comment •

edited

Loading

mvertens commented Nov 3, 2023

TomasTorsvik left a comment

TomasTorsvik Nov 3, 2023

TomasTorsvik commented Nov 3, 2023

jmaerz commented Nov 3, 2023

Some cosmetic changes #299

Some cosmetic changes #299

Conversation

JorgSchwinger commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaerz commented Nov 3, 2023

jmaerz commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaerz commented Nov 3, 2023

jmaerz commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorgSchwinger commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaerz left a comment • edited Loading

Choose a reason for hiding this comment

mvertens commented Nov 3, 2023

TomasTorsvik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTorsvik commented Nov 3, 2023

jmaerz commented Nov 3, 2023

jmaerz left a comment •

edited

Loading