Compile with -std=f2008

[ewu63]

Purpose

This the last remaining repo. This fixes a number of things

• compilation with strict -std=f2008 standard
• compilation under GCC>= 10

I got the real build to work (there are some warnings still but it should run fine). The complex build has some extra problems that will require more investigation, in particular complexify.py probably needs to be modified. I have absolutely no idea how it works in idwarp without such fixes.

I will not continue working on this PR, but this should be done soon as it is blocking adoption of our codes with newer GCC compilers.

Expected time until merged

This is not ready to merge yet.

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Checklist

• I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[codecov]

Codecov Report

Merging #228 (2698301) into main (af5c32b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #228   +/-   ##
=======================================
  Coverage   41.35%   41.35%           
=======================================
  Files          13       13           
  Lines        3794     3794           
=======================================
  Hits         1569     1569           
  Misses       2225     2225           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[eirikurj]

@nwu63 I can have a look at the remaining issues as I wanted to update the complex build anyway.

[ewu63]

Is this ready @eirikurj?

[eirikurj]

Yeah, I think this is good for now. Will mark it ready for review. Few things to note. I created an interface for myisnan to handle both the real and complex checks. C function myisnan is removed in favor of a more modern fortran module. Previously there was not really a check for complex numbers, but here I decided just to check both real and imag parts. This is thus a change in behavior. Also, despite the name myisnan the C function also checked for INF so I retained this behavior in this implementation. Please make sure to test this thoroughly in case you have problems that you can trigger NAN or INF for complex case.

[sseraj]

Looks good. I think this is fine as is so I approved, but I also have some comments:

I have a failing case that produces NaNs in complex mode. The behavior did change with this PR. Instead of crashing with returnFail during the first iteration, it continuously takes NK steps of size 0.01 where all monitor variables are NaNs. I prefer the original behavior, but this is not that big of an issue.

[eirikurj]

I can approve this but, since I made large amount of changes, I would prefer that someone else reviewed, tested, and gave some feedback. @anilyil @joanibal?

[anilyil]

I did want to take a look at this. I can review within the next 2 days if there is no urgency. I wanted to review partly also because my current open PR touches a bit of fortran and I want to see how bad the conflicts will be and if I can fix that PR as well.

[eirikurj]

Ok, from my point of view, that fine. Please take your time testing this, preferably on some complex cases if you have any.

[anilyil]

Most of the changes here look good. I have a few general comments:

1. None of my cases get NaNs right now, so I cannot test this. However, if you want, we can add a test that we force to get a nan and see if we can catch it.
2. What did you mean by try my "complex" cases? As in complicated cases or cases that use complex numbers for complex step checks?
3. Some files only have whitespace added (adBuffer.f, adStack.c). Do we want to keep these whitespace changes or just remove them to have a cleaner diff?
4. Will this now not work with compilation using GCC 9?

Otherwise stuff look good. I don't fully understand the implications of the compilation flags but it would be nice if one of you can give me a quick summary over a call so I dont miss any nuance. The other code changes make sense.

[eirikurj]

@anilyil see my comments below. We can discuss optimization flags offline if you want.

1. I agree that a test would be great. This could be done on several levels. Fortran unit tests for the new genericISNAN.F90 would make sense to test functionality, and then unit or regression tests for larger processes. Did you have something in mind?
2. I wanted to make sure there was no change in behavior, as the C function is now replaced by a fortran module. Previously there was not really a check for complex part, but now both real and imag parts are checked. @sseraj had a case has checked that the behavior is the same for the complex numbers and aggressive optimization flags (see Allow specified modules to be compiled without optimization #233)
3. Based on the commit my guess is that the whitespaces came with a copy-paste from idwarp since the source there had them in. I don't see any need for them and suggest we remove them. I will push some updates.
4. Yes, currently U18 and U20 images use GCC 7.4 and 9.4, respectively.

[joanibal]

Am I correct that this PR is necessary but not sufficient to get ADflow working with GCC 11?

[joanibal]

With a cheeky little -fallow-argument-mismatch I'm able to run it with gcc 11

[joanibal]

Works for me.

[ewu63]

With a cheeky little -fallow-argument-mismatch I'm able to run it with gcc 11

I would like to eventually fix these, so that we do not have to conditionally add those to the Docker builds once we update to U22 which will be using gcc>9. But I guess that does not have to happen in this PR

[sseraj]

@anilyil @eirikurj Any updates on this? We should probably merge this soon.

[eirikurj]

@anilyil wanted to have a chat offline, but we have yet to meet. Will reach out offline so we can figure out a time and can get this merged soon.

[eirikurj]

@sseraj, @anily plans to review this later this week.