Remove #ifdefs in favor of run time logical flags

[DeniseWorthen]

Issue

Currently WW3 uses #ifdefs to control the physics or other configurable features. In modern F90 code, #ifdefs are largely avoided and instead such options are controlled at run time via the use of logical flags. The main disadvantages of #ifdefs are:

1. Compile-time dependence of features or physics options. This reduces flexibility because code must be recompiled to exercise optional features. (In the case of WW3, true run-time control would probably also require modification to the contents of the mod_def file.)

2. Poor Code Readability. Developers cannot easily determine which section of code will execute. The impact is especially acute when the developer encounters nested #ifdefs or branching if-endif logic within an #ifdef.

3. Code becomes unnecessarily long when overused or used when not needed.

4. Testing of all possible permutations of #ifdefs is not possible. Many ifdefs only make sense when other #ifdefs are also set and conflict when a third #ifdef is set.

5. Developers are incentivized to "wall off" their changes instead of utilizing more extensible code. For example, passing an argument under #ifdef control instead of using the optional construct. This results in less extensible code.

Proposal

The use of #ifdefs within WW3 should be minimized to the extent possible. The transition can be accomplished by including a temporary file which would define #ifdefs at compile time to set a matching logical flag. This code would eventually be removed and the logical flags moved to a namelist structure.

The removal of #ifdefs has been automated using a python script developed by @mvertens. This script also leverages emacs to apply standard F90 indenting and syntax. This standardization of syntax will be further explored within the existing discussion #551.

The proposal to remove #ifdefs include the following changes:

1. Remove unneccessary ifdefs. #ifdefs are not required in the following cases

   in most use statements
   in definition of local variables
   in definitions of derived types
   around format statements
   around most write statements
   around OpenMP blocks

2. Reduce granularity of existing "debug" type #ifdefs (eg. debuginit, debugrun etc). Such statements should be retained only at the enter/exit stage of a subroutine and at the enter/exit stage of major code blocks (e.g. "3.1 Interpolate winds and currents"). Highly granular debug statements can be added by developers during debugging but should not be committed.

3. Remove serial #ifdefs of the same type . For example,

#ifdef foo
   ....
#endif  
#ifdef foo
   ....
#endif

In many cases, serial ifdefs only become apparent after nested "debug" ifdefs are replaced with logical flags. Thus, pruning the use of "debug" ifdefs not only allows serial #ifdefs to be removed, it also improves both code readability can significantly reduce code length. For example, in w3wdatmd:

WW3/model/src/w3wdatmd.F90

Line 529 in 67d7715

IF ( FL_ALL ) THEN

Pruning the #ifdef W3_DEBUGINIT lines and collecting the remaining #ifdefs results in

    IF ( FL_ALL ) THEN
       if (w3_debuginit_flag) then
          WRITE(740+IAPROC,*) 'W3DIMW, step 8'
       end if
       ALLOCATE ( WDATAS(IMOD)%VA(NSPEC,0:NSEALM), STAT=ISTAT ); WDATAS(IMOD)%VA = 0.
       CHECK_ALLOC_STATUS ( ISTAT )
#ifdef W3_PDLIB
       ALLOCATE ( WDATAS(IMOD)%VAOLD(NSPEC,NSEAL), stat=istat )
       ALLOCATE ( WDATAS(IMOD)%SHAVETOT(NSEAL), stat=istat )
       WDATAS(IMOD)%SHAVETOT=.FALSE.
       IF (.not. LSLOC) THEN
          ALLOCATE ( WDATAS(IMOD)%VSTOT(NSPEC,NSEAL), stat=istat )
          ALLOCATE ( WDATAS(IMOD)%VDTOT(NSPEC,NSEAL), stat=istat )
          WDATAS(IMOD)%VSTOT=0
          WDATAS(IMOD)%VDTOT=0
       ENDIF ! LSLOC
#endif
       if (w3_debuginit_flag) then
          WRITE(740+IAPROC,*) 'W3DIMW, step 8.10'
          WRITE(740+IAPROC,*) 'NSEAL=', NSEAL, ' NSEALM=', NSEALM
       end if

This equivalent code is more compact (21 lines vs 74) and much more clearly shows what is being done within the PDLIB ifdef.

4. Remove #ifdefs with branching-if logic. These should be replaced with a more extensible construct by defining a local logical to control execution.

For example, in w3wavemd.F90:

WW3/model/src/w3wavemd.F90

Line 3284 in 67d7715

IF ( DTTST .EQ. 0. ) THEN

can be replaced with

....
logical :: do_gridded_output
....

  DO J=1, NOTYPE
   ....
     IF ( FLOUT(J) ) THEN
     .....
           if (w3_sbs_flag) then
              do_gridded_output = ( j .eq. 1 )  .or. ( j .eq. 7 )
           else
              do_gridded_output = ( j .eq. 1 )
           end if
   ....
         IF ( DTTST .EQ. 0. ) THEN
            if (do_gridded_output) then

5. Remove hanging #ifdefs. For example,

WW3/model/src/wminitmd.F90

Line 2834 in 67d7715

#ifdef W3_SHRD

Where the ENDIF at LN 2855 matches either #ifdef W3_SHRD or W3_MPI.

6. Remove or fix #ifdefs which result in compile errors when other #ifdefs are set. For example, during testing it was found that w3meminfo will not compile if W3_S is set.

[MathieuDutSik]

Hello,

I am one of the people that pushed some years ago for the use of #ifdef statements so I thought I ought to comment on that:

• I view #ifdef statements as a necessary evil. It makes the code unclear that is true, but if we want to have fast code, they are useful I believe.
• Removing completely #ifdef statements would require something like making PDLIB mandatory which may be a good idea but something that requires a lot of issues one by one. Putting the choice in a namelist gives the possibility of runtime loss.
• The #ifdef statements are an absolute progress on the previous system, but there is room for further improvement.
• One of the best model to look at for the use of #ifdef statements is the ROMS oceanographic model where things are done very cleanly.
• Some of the statements are indeed bad like "#ifdef COND1 #ifdef COND2" while a better one would be "#if defined COND1 && defined COND2"
• Compile runtime is only a minor annoyance and should not be decisive. I believe the goal should be operational performance.
• Yes #ifdef can be quite complicated to read. But the complexity is not necessarily accidental but exist because of some features. In other words, maybe something should be eliminated to simplify the code.
• If it is about the testing of compilation of code. It is possible to consider many features. The code is not that big. The compilation time is about 1 minute. The maximum runtime of Github actions is 6 hours. A lot of stuff can be tested and tested everyday.
• Passing some function argument under #ifdef statements is indeed not a great style (but does it happen in the WW3 code, I do not know any examples like that). Some rules can be put to that effect. And optional is not necessarily the solution. For example ROMS has the capability to use MASKING of the grid or not (typically no mask would be used for basin scale simulations). Those are real use cases.

About the proposal:

• Putting variables under #ifdef statements makes perfect sense. If a variable is not used, then not having a compilation warning of unused variable is perfectly logical. It is also true that Wavewatch III is not very clean on that but that is another story.
• The same applies to #ifdef in types, import statements and such.
• The loss of granularity with DEBUGINIT and DEBUGRUN is a loss of functionality. I do not really mind some of such statements being eliminated but most were put for valid reasons. I can do some cleanup of that as I am the one who introduced them.
• Proposition 3 "Remove serial #ifdefs of the same type" is completely correct.
• The replacement of "#ifdef W3_DEBUGINIT" by "if (w3_debuginit_flag) then" makes no sense to me. It is potentially slower (an unlikely case, but still) and it has the same code size. I think all the debugging stuff should be if #ifdef statements since by definition the debugger is a programmer and not a user and the user should not have to worry about that.
• Now I agree that the ugly "WRITE(740+IAPROC,*) 'W3DIMW, step 8.7, stat=', istat" has been removed. However, this check addressed a very basic functional need. Checking that memory has been allocated correctly and we ought to have a way to check memory allocations in the program. Many programs such as COSMO (but not ROMS) do systematic check of allocation and I think this is a good thing to have, if only as an option.
• Proposition 4 "Remove #ifdefs with branching-if logic" makes sense to me. But there are many such bad code to clean. It is just bad code, incidentally related to #ifdef statements.
• Proposition 5 "Remove hanging #ifdefs" makes sense to me.
• Proposition 6 "Remove or fix #ifdefs which result in compile errors when other #ifdefs are set" is also good. But I orient myself more toward fix than remove.

Final thoughts:

• Wavewatch is a big boat, incremental improvement is good, but many people are using it and many other are programming in it. Change has to be manageable for them. If suddenly, their "git pull" results in thousands of corrections to do. The perceived improvement might not be so.
• There are many issues in the WW3 code as I hope it is clear to everyone. But #ifdef statements are only one incidental part of it.

[ardhuin]

I believe (in particular for the source term options) that switches and now ifdef was OK as a way to give a lot of freedom : you could do your own things and ignore the other options. I believe that a lot of that could be consolidated, and unnecessary options removed (e.g. we included Romero 2019 as an option within ST4 instead of using "ST5" or "STX" ... )
But that will require some reviewing work.

[mvertens]

@MathieuDutSik - thank you for your thoughtful comments. I'd like to respond to some of the points you raised.

I view #ifdef statements as a necessary evil. It makes the code unclear that is true, but if we want to have fast code, they are useful I believe.

• I agree that there are cases where a minimum set of #ifdefs is a necessary evil - but I believe this is a far smaller number than the number of #ifdefs that are currently in the code. @DeniseWorthen and I have been doing performance profiling with the new logical variables we have introduced and if these variables are set as parameters to replace the #ifdefs there is no performance slowdown that we are seeing. The downside of this is that you then need to recompile the code when you change the logicals. We were seeing about a 2% slowdown without parameters - but the huge upside of that would be that you could test the code in a variety of configurations without recompiling. A possible solution would be to use parameters in production runs and non parameters for testing. But removing the maximum set of #ifdefs from the code allows you to have the flexibility to do that.
• The key point is that if there are too many #ifdefs the code becomes impossible to test. This is already been evident in the fact that when I turned on certain #ifdefs the code would no longer compile.
• Also I would argue that having unclear code gives rise to fragile code that is hard to modify and extend without introducing unexpected errors. Having been the head software engineer for CESM which is a community model used by hundreds of collaborators, code clarity and modularity has become key in terms of ensuring code robustness and extensibility. Of course you also want to ensure that performance is not sacrificed - but by careful code design that can be achieved.

Removing completely #ifdef statements would require something like making PDLIB mandatory which may be a good idea but something that requires a lot of issues one by one. Putting the choice in a namelist gives the possibility of runtime loss.

• As I mentioned above, we could still retain a minimal set of #ifdefs and not make PDLIB mandatory. However, if you still use the scheme that @DeniseWorthen and I are suggesting, you could still have PDLIB be compiled by not activated in the code base. At this point, we are not suggesting the use of namelists - but simply the ability to ensure that the code compiles and runs successfully with a minimal set of #ifdefs.

Proposition 6 "Remove or fix #ifdefs which result in compile errors when other #ifdefs are set" is also good. But I orient myself more toward fix than remove.

• So if you just fix these #ifdefs and then new #ifdefs are added that again are not tested in the regression testing (which is happening now) you are still in the same boat. The more #ifdefs that you can remove - the more robust your testing becomes (since again you cannot ensure that with a large number of #ifdefs you are testing all combinations that can be activated).

Some of the statements are indeed bad like "#ifdef COND1 #ifdef COND2" while a better one would be "#if defined COND1 && defined COND2"

• Agreed. But we found that logical expressions greatly clarified the code since a lot of these multiple #ifdefs actually could be consolidated into several logical expressions that made it clear what the code was trying to do. We'll try to give more examples in our presentation and hopefully that can help convince you of the benefit of the proposed approach.

[MathieuDutSik]

Comments:

• "new #ifdefs are added that again are not tested in the regression testing (which is happening now) you are still in the same boat". Replace "#ifdef" by "fortran" and you get the statement that if you add new code, you potentially add new bugs.
• "are not tested in the regression testing (which is happening now)" So, the solution IS to do the test and to find a solution to the detected problems. That solution may be to remove the #ifdef statements or something else. But you have to consider the problem itself, not presume of the solution from the start.
• "We were seeing about a 2% slowdown without parameters" 2% is not insignificant. We really want a fast model.
• "The key point is that if there are too many #ifdefs the code becomes impossible to test" That is clearly wrong because you found the bug yourself. We need more tests, solutions. That solution MAY be to remove some #ifdef statements.
• "but the huge upside of that would be that you could test the code in a variety of configurations without recompiling" The compilation of the WW3 is not such a big problem. It takes a few minutes. Note also that many choices are hardcoded with ww3_grid which can be seen as another real constraints.

In my view the proposed changes simply focus on some code feature and do not address underlying issues. There is a focus on #ifdef statements to the detriment of real problems and when problems are real, the solution is mandated.
Here are some proposed solutions, which I think would be better.

• More CI tests for various configurations.
• When changing #ifdef statements, it is possible to make sure that files that are not affected are not recompiled. At least there should be a way to do that.
• Single #ifdef statements for ALLOCATE errors. I think this is good to have a way to nicely stop when memory cannot be allocated.
• Remove plenty of debug statements. No objection to that.
• Simplify the statements that can be simplified.
• Debug the bugs that you identified.

[mvertens]

One way that logicals can improve CI performance is that you can run many tests with a single compile. For the purpose of the CI system non-parameter logicals could be used which would enable running many tests with a single compile. this could be done via a single #define of ",parameter" in the shr_wav_flags that we are proposing. This would be hugely beneficial for testing I believe. This would also remove the need to have #ifdefs for allocate errors. With parameters you would not be entering that block of code for the check of the allocation.

[mvertens]

@MathieuDutSik - I've done a quick overview of the number of #ifdefs in CICE6 and MOM6 compared to ww3 and this is what I am seeing:

• MOM6: 124 #ifdefs (about 10 unique ones)
• CICE6: 349 #ifdefs (about 10 unique ones)
• WW3: 9725 #ifdefs (not sure how many unique ones (I see at least 171 unique ones in our new wav_shr_flags)

[MathieuDutSik]

Ok, what about the 25000 ifdef of the ROMS model?
Measuring the number of #ifdef is simply not a reasonable way of evaluating a model.

[DeniseWorthen]

In FV3-ATM (which includes the FV3 dynamic core and CCPP physics), there are about 1000 lines of #ifdef. Of those it appears that about two dozen are unique.

[mvertens]

@MathieuDutSik - #ifdefs greatly increase the cost of testing code which leads to a greatly diminished reliability of the code. Given limited resources for testing I would advocate that this is a drawback of the current WW3 approach. I used examples of modern new code bases that have strongly stayed away from #ifdefs from the onset.

[DeniseWorthen]

Thanks @MathieuDutSik for your engagement and comments. I understand some of your concerns (eg. impact on performance) but I do think the WW3 community should be concerned about more than just operational implementations. Where do improvements to operational implementations come from if not the development community?

My primary area of concern is the coupled applications in the UFS. We provide multiple configurations and applications to the UFS user community ---from standalone FV3 to fully coupled ATM-OCN-ICE-WAV-AEROSOL configurations.

WW3 is the only component for which we cannot have a single compile and then multiple physics/feature tests. We cannot provide the UFS community access to WW3's features, which we must test on an on-going basis, when each flavor of WW3 requires it's own unique application--from compile to mod_def and so on. It is not just about the compile time. Without the capability to easily test multiple flavors of WW3, we simply can't provide the UFS community with a feature rich WW3 component.

I grant you that as newcomers to the code, Mariana and I see WW3 with a different eye. But I think that WW3 does developers a mis-service when code readability is so poor. It really places an undue burden on developers to easily engage with the code. Also, I'm not sure I understand your distinction between developer and user. Shouldn't a user be able to understand what the developer has done? In my opinion, the user is likely to be even more impacted by poor code readability.

In our experience, there are multiple good reasons why other community-based models (even operational ones) rely on a minimum number of #ifdefs.

And, no, it is not just about counting ifdefs---the point was to illustrate the degree to which they're being over-used in WW3 by referencing other complex and feature-rich models which have to a large degree abandoned the ifdef solution.

[sbrus89]

Hi all, just thought I'd offer my two cents:

I don't have experience with operational performance constraints, other than knowing they are very stringent. That being said, a 2% performance difference seems to be within-bounds of the expected variability between runs of the same simulation on a given machine.

It's my general feeling that as long as logic statements are used following reasonable best-practices (e.g not positioned within the inner-most computational loops, no string comparisons, etc.), their performance impact should be extremely low with modern compilers.

[ukmo-ccbunney]

I agree with the comments above regarding having a limited amount of "necessary evil" #ifdefs in the code.
There are likely many places where we can replace the #ifdefs with conditional statements in the code.

Looking at the regression tests, one of the most common differences between test in the same group is changes in the source term package (STx). Perhaps a good starting point would be a refactor of the W3SRCE module to use conditionals rather than #IFDEFS for the source term selection? There are many shared variables in that module and since all the individual source term packages are contained in separate subroutines I think it would have minimal performance impact. Also, it would clean up the W3SRCE module significantly, which would be a really good thing as all the #ifdefs make it a very quite difficult to follow the code...

[aliabdolali]

slides
from Sep 6th Developer call

[sbrus89]

I agree with @ukmo-ccbunney that perhaps source term packages would be a good place to start.

[JessicaMeixner-NOAA]

@DeniseWorthen and @mvertens thank you so much for starting this conversation and your presentation today. To move things forward, we'd ask that comments on this topic be posted by Sunday 9/11/22 so that a path forward can be determined and announced by Wednesday 9/14/22.

[ukmo-ccbunney]

@DeniseWorthen Do you envisage this being made as a single large change, or a piecemeal change over several PRs?
Removing related groups of #ifdefs one at a time should make it easier to pick up bugs and unintended side affects via the regression tests (and tests of operational configs), but of course this will drag the process out a bit...

[DeniseWorthen]

@ukmo-ccbunney I think regardless of how far down the path of ifdef removal path is finally decided up (ie, runtime config), the step which is required regardless is a cleanup. The python script that @mvertens wrote can use a subset of #ifdef statements. So, for example, you could start with just the 'debug' type ifdefs. You could stage the changes over several PRs by dividing the files into groups for example. There is definitely a trade-off between limiting changes and dragging out the process.

Now, converting the 3 lines of an #ifdef to three lines of if(..flag) gets you only so far. But when we were prototyping this process, once many of these "debug" ifdefs got converted, the serial and other granular ifdefs become much more visible. It was much easier to spot and do clean-up at that point. But that is essentially a manual process; some files have a lot of required clean-up and others not much. I do think it is really important to set out ahead of time the desired level of debug-type statements.

The other comment I'd make about staging, is that the overall process is much easier once the indenting issue is fixed. When you're trying to clean up ifdefs, it can get very difficult over 100s of lines if the endif you're hunting for isn't where you expect it to be. That is how these two issues (ifdefs, code standards) became somewhat linked.

So, in my perfect world, the F90-free format + indenting changes + unwrapping lines (which would be almost all whitespace changes) would come in first. In our experience that makes the next step much easier. That next step would be primarily an ifdef-cleanup via the use of logical flags for a sub-set of all ifdefs, most like in chunked file sets.

[stefanzieger]

Hi there, I am in favour of reducing the number of ifdefs to a minimum and the best way to start is with source terms as pointed out by Chris Bunney. Beyond that, I think a code review is necessary because from experience some switched won't work or are compatible with other. For example SMC, ARCTIC and PDLIB for unstructured mesh will generate a different code base and won't work in ww3_multi as intended with rectilinear , tripolar, curvilinear grids.
The place where I can see ifdefs useful is when it comes to MPI and the distribution among processes. I might me wrong here and there could well be a common solution to this. It would also be good to have OpenMP instructions enabled so that threading could be utilised (when possible) for supporting tools.

[ukmo-ccbunney]

@stefanzieger Yes - I did wonder how easy it would be to have the SHRD and MPI functionality of WW3 coexisting as runtime options. I think your concerns regarding the different grid types are justified too. I've not doubt that some code refactoring will be needed in that latter case!

[aliabdolali]

I second @stefanzieger's comment.

[JessicaMeixner-NOAA]

For those concerned about the disruption of the a potential large change and how that impacts their own development work: We will give a minimum of two weeks notice to allow those with ongoing work to submit PRs (similar to when we went from switches to ifdefs). If you need more than 2 weeks, please let us know and we can start to plan and work to minimize the disruption to development.

[JessicaMeixner-NOAA]

In this vein, @mvertens and @DeniseWorthen would it be possible to make the conversion scripts available so those who might have substantial development that is not ready for merge prior to these change, so that they could convert their own codes to assist their development with this potential development?

[DeniseWorthen]

Yes, of course. @mvertens is on leave right now, but I believe this is a current version (attached). I had to gzip it in order to attach it here, but it is just a python script.

It can be executed like so:

python3 remove_ifdefs.py --input w3initmd.F90

remove_ifdefs.py.gz

The final step (the emacs command) will notify if unclosed if or do loops are found. Those need to be resolved manually.

[JessicaMeixner-NOAA]

There is support for moving forward with this proposal. There seem to be a few major concerns:

• Computational performance impacts
• A few switches may remain necessary (eg PDLIB)
• Disruption of code

However, the benefits of this change outweigh the cons. To address the major concerns we will do the following:

• Target is to have no more than a 5% performance hit on the computational speed of the code (compared with no if-defs converted) to accept these additions. It is expected that if there is more than a 5% hit, that restructuring ifs and do loops should be able to off-set this impact. Any work to restructure the code to achieve needed performance gains should be addressed in separate PRs before conversion from #ifdef to if statements.
• Recommend to convert related subsets of switches in batches instead of converting all switches at once, with most recommending to start with the source term switches.
• A two week notice period will be provided to allow those with ongoing work time to submit PRs with their work. Any PR submitted in this time will be in the queue before the other work. Additionally, the scripts to convert the code will be provided if anyone needs to convert their own code (similar to the WW3 switch to CCPP transition).

Testing of a PR with related updates:

• Normal testing of the WW3 PRs is running the matrix of standalone WW3 regression tests. However, since this is such a large change and there is expectation that there will be performance impacts and it is possible to not be able to see major performance issues on the WW3 regression tests, all community code managers will be asked to review if-def to if PRs with their regular tests as well as testing their larger set-ups that are not included in the regression tests to ensure that we avoid any surprises in terms of performance impacts that are not seen with the typical regression tests.

[MathieuDutSik]

I agree with the need for restructuring the #ifdefs and I wrote a PR about that that simplifies a lot of them and I hope it will be merged into the develop branch soon. I would think that doing incremental changes are a reliable way to go forward and I have other proposals.

I must say that a 5% performance hit being acceptable is a revelation to me. We will see how things evolve.

[JessicaMeixner-NOAA]

@MathieuDutSik I agree I'd like to see your PR get to the develop branch, ASAP. I'll work with @thesser1 on Monday to make sure that happens early next week.

The 5% threshold was chosen as it's impossible to think that there will be no impact and weighing the pro's with the con's. Even that 5% impact could likely be reduced by restructuring of if's that appear in loops and we hopefully we can all work together to address any performance issues that might arise. Please note that early testing indicates 1-2% and I also suspect the work you did to eliminate a lot of debug statements will help reduce this.