remove most hamocc #ifdefs in favorite of new logicals #264

mvertens · 2023-08-01T12:51:42Z

This is a draft PR that is a prototype implementation to move away from #ifdefs in BLOM and particularly in HAMOCC to the greatest extent possible. Most modeling efforts have adopted the approach to have the least number of #ifdefs possible.
This is a result of the fact that the code itself is not really tested until all possible #ifdef combinations are invoked. The more #ifdefs there are - the greater is the challenge to do this. By minimizing #ifdefs - the compilation can at least detect if there are inconsistencies in various branches that would always be compiled regardless of if they are actually invoked run time.

The approach I am suggesting is to introduce a new module in hamocc - mo_ifdefs.F90 that contains logicals that are parameters and that are triggerred by the presence of the #ifdefs.

#ifdef BROMO
  logical, parameter :: use_BROMO = .true.
#else
  logical, parameter :: use_BROMO = .false.
#endif

In the code itself - the majority of the #ifdefs are then replaced with logicals like (e.g. in aufw_bgc.F90):

       if (use_BROMO) then
         CALL write_netcdf_var(ncid,'bromo',locetra(1,1,1,ibromo),2*kpke,0)
      end if

Note that the work around to deal with ibromo values of -1 (when #BROMO is not defined) is to define loctre as

      REAL :: locetra(kpie,kpje,2*kpke,-1:nocetra) ! local array for reading

But to initialize locetra as

      locetra(:,:,:,1:)=trc(1:kpie,1:kpje,:,itrbgc:itrbgc+ntrbgc-1)

Multiple compilation errors were found when this was implemented. Below is a brief summary

The following were not defined when the ifdefs were removed - which means they would not have worked if the ifdefs were turned on

beleg_vars.F90(254): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC13]
beleg_vars.F90(255): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC14]
beleg_vars.F90(256): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO13]
beleg_vars.F90(257): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO14]
beleg_vars.F90(258): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC13]
beleg_vars.F90(259): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC14]

carchm.F90(636): error #6404: This name does not have a type, and must have an explicit type.   [KS]
carchm.F90(642): error #6410: This name has not been declared as an array or a function.        [SEDLAY]
carchm.F90(643): error #6410: This name has not been declared as an array or a function.        [POWTRA]
carchm.F90(644): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO14]
carchm.F90(645): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC14]
carchm.F90(646): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC14]

aufw_bgc.F90(891): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO13]
aufw_bgc.F90(892): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO14]
aufw_bgc.F90(893): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC13]
aufw_bgc.F90(894): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC14]
aufw_bgc.F90(895): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC13]
aufw_bgc.F90(896): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC14]
aufw_bgc.F90(907): error #6404: This name does not have a type, and must have an explicit type.   [IATMC13]
aufw_bgc.F90(908): error #6404: This name does not have a type, and must have an explicit type.   [IATMC14]
aufw_bgc.F90(911): error #6404: This name does not have a type, and must have an explicit type.   [IATMNCO2]

aufr_bgc.F90(483): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO13]
aufr_bgc.F90(484): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO14]
aufr_bgc.F90(485): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC13]
aufr_bgc.F90(486): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC14]
aufr_bgc.F90(487): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC13]
aufr_bgc.F90(488): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC14]
aufr_bgc.F90(502): error #6404: This name does not have a type, and must have an explicit type.   [IATMC13]
aufr_bgc.F90(503): error #6404: This name does not have a type, and must have an explicit type.   [IATMC14]
aufr_bgc.F90(509): error #6404: This name does not have a type, and must have an explicit type.   [PREI13]
aufr_bgc.F90(511): error #6404: This name does not have a type, and must have an explicit type.   [PREI14]
aufr_bgc.F90(524): error #6404: This name does not have a type, and must have an explicit type.   [IATMNCO2]
aufr_bgc.F90(591): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC]
aufr_bgc.F90(593): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO]
aufr_bgc.F90(595): error #6404: This name does not have a type, and must have an explicit type.   [ISSSC]

accfields.F90(195): error #6404: This name does not have a type, and must have an explicit type.   [JATMO2]
accfields.F90(196): error #6404: This name does not have a type, and must have an explicit type.   [JATMN2]

ncout_hamocc.F90(482): error #6404: This name does not have a type, and must have an explicit type.   [JATMO2]
ncout_hamocc.F90(482): error #6404: This name does not have a type, and must have an explicit type.   [SRF_ATMO2]
ncout_hamocc.F90(483): error #6404: This name does not have a type, and must have an explicit type.   [JATMN2]
ncout_hamocc.F90(483): error #6404: This name does not have a type, and must have an explicit type.   [SRF_ATMN2]

aufr_bgc.F90(133): error #6580: Name in only-list does not exist or is not accessible.   [IPOWC]
aufr_bgc.F90(133): error #6580: Name in only-list does not exist or is not accessible.   [ISSSO]
aufr_bgc.F90(133): error #6580: Name in only-list does not exist or is not accessible.   [ISSSC]

aufw_bgc.F90(125): error #6580: Name in only-list does not exist or is not accessible.   [IPOWC]
aufw_bgc.F90(125): error #6580: Name in only-list does not exist or is not accessible.   [ISSSO]
aufw_bgc.F90(125): error #6580: Name in only-list does not exist or is not accessible.   [ISSSC]
aufw_bgc.F90(126): error #6580: Name in only-list does not exist or is not accessible.   [JATMO2]
aufw_bgc.F90(126): error #6580: Name in only-list does not exist or is not accessible.   [JATMN2]
aufr_bgc.F90(134): error #6580: Name in only-list does not exist or is not accessible.   [JATMO2]
aufr_bgc.F90(134): error #6580: Name in only-list does not exist or is not accessible.   [JATMN2]
aufw_bgc.F90(680): error #6404: This name does not have a type, and must have an explicit type.   [RMASKS]
aufr_bgc.F90(527): error #6404: This name does not have a type, and must have an explicit type.   [IATMNCO2]
aufr_bgc.F90(594): error #6404: This name does not have a type, and must have an explicit type.   [IPOWC]
aufr_bgc.F90(596): error #6404: This name does not have a type, and must have an explicit type.   [ISSSO]

The following is a bug in aufr_bgc - ipowc does not exist
      powtra2(i,j,k,ipowc13)=powtra2(i,j,k,ipowc)*rco213*bifr13
      powtra2(i,j,k,ipowc14)=powtra2(i,j,k,ipowc)*rco214*bifr14

The following is also a bug in aufr_bgc - issso and isssc do not exist
      sedlay2(i,j,k,issso13)=sedlay2(i,j,k,issso)*rco213*bifr13
      sedlay2(i,j,k,issso14)=sedlay2(i,j,k,issso)*rco214*bifr14
      sedlay2(i,j,k,isssc13)=sedlay2(i,j,k,isssc)*rco213
      sedlay2(i,j,k,isssc14)=sedlay2(i,j,k,isssc)*rco214

in hamocc4bcm.F90 - there is the following use statement but no corresponding module or call
      use mo_boxatm,      only: update_boxatm ERROR: there is no mo_boxatm
      if (use_BOXATM) then
         CALL update_boxatm(kpie,kpje,pdlxp,pdlyp)
      end if

NOTE: I realize that this is an invasive PR and as such maybe its not the time to do it. But I wanted to get the idea out there regarding the downside of an #ifdef based approach.

NOTE: The hamocc arrays are still not being dynamically allocated - but are based on parameter values as before in this implementation.

…mediator OR send the fluxes back

…assed back from mediator

…_flux_frmed

jmaerz · 2023-08-02T11:41:05Z

Dear @mvertens , @TomasTorsvik , @JorgSchwinger , as announced in the PM, we discussed the pull requests and the potential order of bringing in all the exciting new techniques and developments into iHAMOCC/BLOM. Since this pull request (#264) is quite invasive: @mvertens , would you be ok, if we would first i) add the boxatm routine, ii) potentially make minor changes in the sedshi.F90, routine and iii) , currently most useful, pull in #261 so that you would have to update this pull request (#264), before eventually merging it into master? We would let you know, once the master branch is prepared for an update of #264.

We further discussed that the if(use_...) statements for the initialization of parameters in most cases could be left out eventually, once the ifdefs are removed, so that in the new module mo_parambgc_ini simply some lines need to be removed when adopting the run-time solution for the pre-processor flags.

jmaerz · 2023-08-02T11:53:35Z

hamocc/aufw_bgc.F90


-
+      use mo_ifdefs


Here and potentially in other routines: it would be nice to explicitly incorporate all used switches, coming from mo_ifdefs (or potentially later from mo_control_bgc).

In the latest changes I have pushed back there is no long the mo_ifdefs.F90 routine - all the logic has been migrated to mo_control_bgc.F90.

mvertens · 2023-08-02T12:31:10Z

@jmaerz- your plan sounds very good. However, we need to bring in #263 and #259 before we bring in #264.
Below is a new summary - with a question regarding 1.

new PR: add the boxatm routine and potentially make minor changes in the sedshi.F90. (DONE)
bring in Feature iHAMOCC nml #261 (DONE)
bring in refactor buildnml to use the CIME-CCS python based namelist generation capabilities #263
- create baselines for namelist refactor.
bring in New capability to have BLOM optionally use DMS and BROMO fluxes computed in mediator #259
- with the default being to keep the code bfb by setting do_bgc_aofluxes=.true.
- verify that results are bfb with baselines created in 4.)
bring in remove most hamocc #ifdefs in favorite of new logicals #264
- verify that results are bfb with baselines created in 4.
create new PR where do_bgc_aofluxes = .false.
- have the mediator compute dms and bromo fluxes - this will also require a new CMEPS tag AND a new noresm snapshot to pull in the new CMEPS tag for creating the DMS and BROMO fluxes.
- create new baselines.
- introduce new namelist capability for do_bgc_aofluxes.

JorgSchwinger · 2023-08-02T12:40:31Z

dd the boxatm routine and potentially make minor changes in the sedshi.F90, routine - what PR would this be in???

I am preparing a PR based on current master with mo_boxatm added right now. If I encounter no further problems, this PR will be available in a few hours.

mvertens · 2023-08-02T13:00:53Z

@JorgSchwinger - thanks for the clarification. I have updated my comment.

JorgSchwinger · 2023-08-03T19:38:15Z

new PR: add the boxatm routine and potentially make minor changes in the sedshi.F90.

With merging #265 and #266 this milestone has been reached.

mvertens · 2023-08-04T07:45:08Z

@JorgSchwinger - thanks so much for your work on 1.

jmaerz · 2023-08-09T13:21:06Z

Merge #261 has now also been carried out - so point 2 of the above list is also done.

JorgSchwinger · 2023-09-27T08:19:18Z

Dear @mvertens, @TomasTorsvik , and @jmaerz

This PR comes with a lot of fixes for missing variables in only lists. I'm currently trying to get current master up to date for running C-isotopes with sediment switched on (we want this for the upcoming release), so I need to put in many of these fixes that are here already.

This is fine for me, but it will certainly lead to a bunch of merge conflicts when we want to merge this branch in. Any thoughts or advise on this?

jmaerz · 2023-09-27T11:38:59Z

Hi @JorgSchwinger , @mvertens and @TomasTorsvik , if I got you right during talking to you over lunch, @JorgSchwinger , you would want to merge your changes for the carbon isotopes into master (currently, there is no pull request for this, which should come soon - probably within this week from @monsieuralok ). I suspect that there will be some work needed to bring the ifdefs branch to a state that it can be easily merged with master (due to a number of merge conflicts). I would suggest to update the ifdefs branch (merge master into ifdefs to get a common hook) once the master includes the updates for the carbon isotopes. Then perform the pull request for #264 after a final round of review. Hence the question to @mvertens , if you would be willing to do it this way or do you see/suggest to perform different steps?

Further, since #259 is also pending, the above scenario probably also holds for #259, which, if I understand it correctly, shall be pulled in before this pull request (#264). Since it is thus far a recurrent discussion issue: both #259 and #264 should not affect compatibility with MCT - somehow, we have to ensure this for the current version and at least until the INES interim release.

mvertens · 2023-09-27T11:52:15Z

@JorgSchwinger - please go ahead and bring your changes in. I am fine handling the merge conflicts when we decide to bring this in.

In addition, I need to create baselines for the current code so that upcoming PRs can test against it. I should have done this as part of the previous PR - but got pulled into working for integrating the oslo aerosols code into the latest version of Noresm/CAM. I'm trying to generate those baselines now - but am running into some strange failures for some of the tests. I'll provide an update once I know more.

TomasTorsvik · 2023-10-11T11:34:45Z

@mvertens - would it be possible to merge this PR into BLOM master without doing #259 first? I was discussing with Jörg and Jöran today, and we see that removing CPP flags could be useful to have in the upcoming release, but we were wondering if this could somehow be done independently of the DMS and BROMO development.

mvertens · 2023-10-11T12:29:51Z

@TomasTorsvik - great idea. I think that's the right thing to do. Do you want this PR to go next? If so - let me work on this over the next day or two and make this PR no longer draft. I've created baselines for the previous tag - so it should be easy for me to validate that this will not change answers.

mvertens · 2023-10-16T07:33:23Z

The PR is now closed in favor of #280 and #281.

mvertens and others added 30 commits June 8, 2023 13:45

first set of updates to send and receive dms fluxes

fa648b7

more updates to update ocetra with mediator fluxes

47700e5

first working version of being able to have blom use dms fluxes from …

996903b

…mediator OR send the fluxes back

fixes to have dms tracer update occur correctly when dms fluxes are p…

129e1cb

…assed back from mediator

put in changes necessary for backwards compatibility and testing

670d3d5

fixes for slowdown

a8540ea

updated to blom master

650253d

minor updates

714a4e0

reimplementation for PR

8c4fa29

fix compute_flxdms logic

42d95f7

simplification of flxdms setting

c4bc745

changed compute_flxdms to do_bgc_aofluxes

b964a51

these changes cause nans for Faox_dms back from mediator

2f1cc53

added bromo logic

3ccf919

Merge branch 'feature/dms_flux_frmed_pr_bromo_debug' into feature/dms…

ebdca5a

…_flux_frmed

changes to get bromoform computed correctly by mediator

c18571c

first steps in refactoring buildnml

d3eb11b

updates to refactor addition of bromo

5957d2c

rename files

b396a60

updates for python namelist generation

8638c67

more updates

46bfd6a

more cleanup

4da982e

udpates for bgcnml

e99c590

more extensions

c7c6c6e

update for bgc diag output

ab237bd

updates for user_nl_blom and copy of ocn_in.readme

4bac767

cleanup of python code

183bbf8

replaced double with single quotes

9f2e803

updates as part of testing

417923e

more bug fixes from testing

bfdf804

implemented issues raised in the PR review

a65eaff

jmaerz mentioned this pull request Aug 2, 2023

Feature iHAMOCC nml #261

Merged

removed BROMO ifdef

8b39950

jmaerz reviewed Aug 2, 2023

View reviewed changes

mvertens added the enhancement New feature or request label Aug 2, 2023

JorgSchwinger mentioned this pull request Aug 2, 2023

Add file mo_boxatm.F90 #265

Merged

mvertens added 2 commits August 3, 2023 09:11

fixed comment

46b589c

updated to master

e0e1c83

JorgSchwinger mentioned this pull request Aug 3, 2023

Add capability to run C-isotopes with sediment code switched on #266

Merged

This was referenced Aug 9, 2023

Merge extended nitrogen cycle, M4AGO scheme and preformed Si tracer into master #269

Merged

New feature: Implementation of an extended nitrogen cycle in iHAMOCC #147

Closed

jmaerz mentioned this pull request Aug 18, 2023

refactor buildnml to use the CIME-CCS python based namelist generation capabilities #263

Merged

jmaerz mentioned this pull request Sep 27, 2023

Fix missing variables in use statements for C-isotope code #276

Merged

NorESMhub deleted a comment from tomas Oct 11, 2023

mvertens mentioned this pull request Oct 13, 2023

remove #ifdefs from HAMOCC #280

Merged

mvertens closed this Oct 16, 2023

mvertens deleted the feature/refactor_ifdefs branch May 15, 2024 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove most hamocc #ifdefs in favorite of new logicals #264

remove most hamocc #ifdefs in favorite of new logicals #264

mvertens commented Aug 1, 2023 •

edited

Loading

jmaerz commented Aug 2, 2023 •

edited

Loading

jmaerz Aug 2, 2023

mvertens Aug 2, 2023

mvertens Aug 3, 2023

mvertens commented Aug 2, 2023 •

edited

Loading

JorgSchwinger commented Aug 2, 2023

mvertens commented Aug 2, 2023

JorgSchwinger commented Aug 3, 2023

mvertens commented Aug 4, 2023

jmaerz commented Aug 9, 2023

JorgSchwinger commented Sep 27, 2023

jmaerz commented Sep 27, 2023

mvertens commented Sep 27, 2023

TomasTorsvik commented Oct 11, 2023

mvertens commented Oct 11, 2023 •

edited

Loading

mvertens commented Oct 16, 2023

remove most hamocc #ifdefs in favorite of new logicals #264

remove most hamocc #ifdefs in favorite of new logicals #264

Conversation

mvertens commented Aug 1, 2023 • edited Loading

jmaerz commented Aug 2, 2023 • edited Loading

jmaerz Aug 2, 2023

Choose a reason for hiding this comment

mvertens Aug 2, 2023

Choose a reason for hiding this comment

mvertens Aug 3, 2023

Choose a reason for hiding this comment

mvertens commented Aug 2, 2023 • edited Loading

JorgSchwinger commented Aug 2, 2023

mvertens commented Aug 2, 2023

JorgSchwinger commented Aug 3, 2023

mvertens commented Aug 4, 2023

jmaerz commented Aug 9, 2023

JorgSchwinger commented Sep 27, 2023

jmaerz commented Sep 27, 2023

mvertens commented Sep 27, 2023

TomasTorsvik commented Oct 11, 2023

mvertens commented Oct 11, 2023 • edited Loading

mvertens commented Oct 16, 2023

mvertens commented Aug 1, 2023 •

edited

Loading

jmaerz commented Aug 2, 2023 •

edited

Loading

mvertens commented Aug 2, 2023 •

edited

Loading

mvertens commented Oct 11, 2023 •

edited

Loading