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

Feature code cosmetics2 #308

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Feature code cosmetics2 #308

merged 5 commits into from
Nov 14, 2023

Conversation

JorgSchwinger
Copy link
Contributor

This is my last contribution before we make a last CMIP6 backwards-compatible release. It is mostly cosmetics. I also combined three modules with netcdf routines into one (named mo_netcdf_bgcrw).

@@ -18,15 +18,15 @@

module mo_control_bgc

Copy link
Collaborator

@jmaerz jmaerz Nov 11, 2023

Choose a reason for hiding this comment

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

Hi Jörg,
first of all many thanks for your effort!
Would you mind adding comments, in which namelist the logicals can be changed? - I think it would be good for understanding their role and the function for HAMOCC.

I'll have a more detailed look into all your changes latest on Tuesday afternoon.

Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

Thanks @JorgSchwinger ! It's nice to have the netcdf routines in a single module.

@TomasTorsvik TomasTorsvik added the iHAMOCC Issue mainly concerns the iHAMOCC code base label Nov 13, 2023
@TomasTorsvik TomasTorsvik added this to the NorESM2.1 milestone Nov 13, 2023
ocetra(i,j,1:kmle(i,j),idet14) = ocetra(i,j,1:kmle(i,j),idet14) + &
ocetra(i,j,1:kmle(i,j),idet14)/(ocetra(i,j,1:kmle(i,j),idet)+safediv)* &
rivin(i,j,irdet)*fdt/volij
ocetra(i,j,1:kmle(i,j),isco213) = ocetra(i,j,1:kmle(i,j),isco213) + &
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @JorgSchwinger and @TomasTorsvik , here and throughout the document: when we do line continuation: what is good practice in terms of where to put the math symbol? - thus far, I personally preferred to have it in the next line, as in (pseudo-code):

ocetra = ocetra*xxx &
               + yyy

to know, what is happening with the stuff in the continuation line. But is there common agreement on this? - at the moment, I see a mixed approach... - not sure, if we should change this, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, but I tend to put the math operator first on a new line and not last on the preceding line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change this here (first in the new line), since it is a mix here, but I won't go through the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. Fair enough. As written, these things I feel as rather minor, anyway.

logical :: with_dmsph = .false. ! apply DMS with pH dependence
integer :: sedspin_yr_s = -1 ! start year for sediment spin-up
integer :: sedspin_yr_e = -1 ! end year for sediment spin-up
integer :: sedspin_ncyc = -1 ! sediment spin-up sub-cycles
character(len=64) :: ocn_co2_type
Copy link
Collaborator

Choose a reason for hiding this comment

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

One could set a comment, which values ocn_co2_type can hold (and what it is good for).

@@ -143,7 +144,7 @@ subroutine dipowa(kpie,kpje,kpke,omask,lspin)
l = ks-k
do i = 1,kpie
if (omask(i,j) > 0.5) then
powtra(i,j,l,iv) = ( sedb1(i,l,iv) &
powtra(i,j,l,iv) = ( sedb1(i,l,iv) &
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't fit in one line? - but maybe its just a bit over the line - not sure, if we should see this a bit sloppy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, doesn't fit. Since it was like this before, I left it.

real, intent(in) :: rivin (kpie,kpje,nriv) ! riverine input [kmol m-2 yr-1].
real, intent(in) :: ndep (kpie,kpje) ! nitrogen deposition [kmol m-2 yr-1].
real, intent(in) :: oafx (kpie,kpje) ! alkalinity flux from alkalinization [kmol m-2 yr-1]
real, intent(in) :: pi_ph (kpie,kpje)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Jörg, can you add a comment, what pi_ph is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -237,7 +233,7 @@ subroutine hamocc4bcm(kpie,kpje,kpke,kbnd,kplyear,kplmon,kplday,kldtday,&
endif

call carchm(kpie,kpje,kpke,kbnd,pdlxp,pdlyp,pddpo,prho,pglat,omask, &
Copy link
Collaborator

Choose a reason for hiding this comment

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

linebreak needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fits in a single line indeed!

Copy link
Collaborator

@jmaerz jmaerz left a comment

Choose a reason for hiding this comment

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

Hi @JorgSchwinger , many thanks again to go through the code that carefully to check the previously done automatic indentation, etc. Perceive the most comments that I was doing as options and address them or not (as you like). I guess, the only thing, where I would be in favor of, would be to break the line before operators. But we can also keep it as mixed as it is at the moment. Feel free to push it as it is at the moment.

@JorgSchwinger JorgSchwinger merged commit 24db948 into NorESMhub:master Nov 14, 2023
@JorgSchwinger JorgSchwinger deleted the feature-code_cosmetics2 branch November 14, 2023 15:31
@jmaerz
Copy link
Collaborator

jmaerz commented Nov 14, 2023

Many thanks! - I'll make the "HAMOCC ifdefs" ready the next hour to enable compiling BLOM without iHAMOCC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iHAMOCC Issue mainly concerns the iHAMOCC code base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants