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 hamocc sediment spinup #162

Merged
merged 5 commits into from
May 18, 2022
Merged

Feature hamocc sediment spinup #162

merged 5 commits into from
May 18, 2022

Conversation

JorgSchwinger
Copy link
Contributor

@jmaerz I am not able to request a review from you. Maybe you can have a look nevertheless (and probably you need to be added by someone to something that you can be requested as reviewer?).

Conflicts:
	hamocc/accfields.F90
	hamocc/hamocc_init.F
	hamocc/inventory_bgc.F90
	hamocc/mo_bgcmean.F90
…compatibility. A change to the correct value will require some re-tuning.
@JorgSchwinger
Copy link
Contributor Author

I have tested for bit-identical results for the NOINYOC compset on the T62_tn21 grid (OMIP1 simulation) and a fully coupled NorESM2 1%-CO2 simulation.

By the way, the previous BLOM version that is in the master currently wouldn't compile for fully coupled compsets (variable "atm" wasn't included in use statement in hamocc4bcm) - the automated testing doesn't seem to catch such errors?

@jmaerz
Copy link
Collaborator

jmaerz commented May 12, 2022

Hi @JorgSchwinger , I'll try to have a look into this soon (being a bit time constrained). With regards to the compilation error: thanks for catching this. I suspect, the automated testing might not have included the right pre-processor flag(s) which would have enabled to catch this error, but I haven't looked into this, thus far. For other cases, NorESM was compiling.

Conflicts:
	cime_config/buildnml
	hamocc/hamocc_init.F
	hamocc/mo_control_bgc.F90
@TomasTorsvik TomasTorsvik added enhancement New feature or request iHAMOCC Issue mainly concerns the iHAMOCC code base labels May 12, 2022
@TomasTorsvik
Copy link
Contributor

inventory_bgc.F90 is included in the Files changed list, but only has whites pace changes. Did something get lost in the merging? Otherwise I think this file can be checked out from master and removed from this PR.

@TomasTorsvik
Copy link
Contributor

TomasTorsvik commented May 12, 2022

The save attributes in mo_control_bgc.F90 are redundant, so this only has a stylistic function. Maybe it can be useful to see the save attribute if you run a grep command on the source code, but otherwise I don't see a reason for doing it this way (as a general code style).

@@ -173,7 +175,7 @@ SUBROUTINE HAMOCC4BCM(kpie,kpje,kpke,kbnd,kplyear,kplmon,kplday,kldtday,&
#endif

#ifdef BROMO
!$OMP PARALLEL DO
!$OMP PARALLEL DO PRIVATE(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate openMP bugfix?

@@ -353,7 +377,7 @@ SUBROUTINE HAMOCC4BCM(kpie,kpje,kpke,kbnd,kplyear,kplmon,kplday,kldtday,&
!--------------------------------------------------------------------
! Pass bromoform flux. Convert unit from kmol CHBr3/m^2 to kg/m^2/s.
! Negative values to the atmosphere
!$OMP PARALLEL DO
!$OMP PARALLEL DO PRIVATE(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate openMP bugfix?

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 agree, I should have done this in a separate commit, but now I don't have the time to disentangle this so I'll leave it as it is.

@TomasTorsvik
Copy link
Contributor

The openMP bug fixes seem to be unrelated to the sediment spinup. Would it make sense to do this in a separate commit / PR? Probably this should also be included as a bug fix to the release-1.2 branch.

@@ -105,6 +106,9 @@ subroutine powach(kpie,kpje,kpke,kbnd,prho,omask,psao)
! needed for boundary layer vertilation in fast sediment routine

real :: bolven(kpie)

! Set array for saving diffusive sediment-water-column fluxes to zero
sedfluxo(:,:,:) = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you set sedfluxo to zero every time powach is called, but in dipowa this is written as something should be accumulated.

Comment on lines 212 to +213
sedfluxo(i,j,iv) = sedfluxo(i,j,iv) &
& + ocetra(i,j,kbo(i,j),iv) - aprior
& -(ocetra(i,j,kbo(i,j),iv) - aprior)* bolay(i,j)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be without accumulation in sedfluxo?

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 think this is correct as it is: In powach sedfluxo is set to zero when entering the sediment. Then it is receiving a value for silicate and oxygen (but not the others) in powach. After that dipowa is called, where sedfluxo is updated in a loop. In order to avoid overwriting the values for oxygen and silicate, it is written as above.

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, Jörg, I think this looks very good! I have left some comments that you may want to address, but probably it can also be merged as is and we can deal with these things afterwards.

@JorgSchwinger
Copy link
Contributor Author

Regarding the save attributes in modules: It's not true that they are redundant. The Fortran standard only guarantees that the values of module variables are "saved" if a variable is referenced in the compiled main program (same as for COMMON blocks).

I don't think that any compiler would NOT save the values of module variables that are not referenced in main, but strictly speaking this is necessary (as far as I know, maybe my knowledge is outdated).

@JorgSchwinger JorgSchwinger merged commit 221f323 into NorESMhub:master May 18, 2022
@JorgSchwinger JorgSchwinger deleted the feature-hamocc_sediment_spinup branch May 18, 2022 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iHAMOCC Issue mainly concerns the iHAMOCC code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants