Compilation on GCC 11

[ewu63]

Purpose

I fixed a few things in the source code such that it can be compiled by gcc 11.

• The main thing to fix was rank mismatch between 1D array and scalar. To fix this, I converted them to singleton arrays and differentiated with Tapenade
• I deleted one file in outputReverse that I believe is unused, since the same code exists in kd_tree which is specifically hand-differentiated.

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

Both real and complex tests now pass.

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 #61 (c3e5e56) into master (07a0f97) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   45.80%   45.80%           
=======================================
  Files           6        6           
  Lines         751      751           
=======================================
  Hits          344      344           
  Misses        407      407           

Impacted Files	Coverage Δ
idwarp/__init__.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31ebbd9...c3e5e56. Read the comment docs.

[bernardopacini]

Overall this looks good to me. Do you know of possible edge-cases where these changes would compile with GCC 11 but not work at runtime?

[ewu63]

Do you know of possible edge-cases where these changes would compile with GCC 11 but not work at runtime?

Not that I know of, and we do not test against GCC 11 anyways (I think we are on 8/9 in the testing images) so I think this is fine. I am running GCC 11 locally which is the motivation behind this PR.

[sseraj]

I'm not too familiar with IDWarp or Tapenade, but the changes look fine to me.

[ewu63]

@bernardopacini or @anily could you take a look at this PR?

[anilyil]

Sorry for the late reply, before we merge this, can someone add a comment in the code about how that routine that looks like its differentiated by tapenade is actually hand differentiated (or at least modified by hand etc)? Also, was there no way around using scalars instead of a single entry arrays to get this working?

[bernardopacini]

Sorry for the late reply, before we merge this, can someone add a comment in the code about how that routine that looks like its differentiated by tapenade is actually hand differentiated (or at least modified by hand etc)? Also, was there no way around using scalars instead of a single entry arrays to get this working?

For the second part here, that can be done using the fallow-argument-mismatch flag when compiling. The problem is, this flag will error out when using GCC 9 and older as it does not exist in previous versions before 10. If we do want to stick with this approach, we could make a new configuration file that adds this flag (this is what I do in my codes when I have to use Tapenade). @nwu63 What do you think? In retrospect, this might be a good solution that doesn't require acrobatics in the code?

[anilyil]

Would that flag hide any other inconsistencies when adding new code?

[bernardopacini]

Would that flag hide any other inconsistencies when adding new code?

It doesn't hide them, it just converts the mismatch errors into warnings. It does make it easier to mistakenly pass a scalar for a 1 element array (what we are trying to allow here), or something like that, but you will at least be warned on compile time.

[anilyil]

Okay, that makes sense. I don't want to delay this PR any further for that "nice to have". This is definitely code gymnastics that we probably don't want, but the code probably already has tons of stuff like this. Ultimately, it's up to you. I think there is value in fixing this w/o arrays, and I am surprised there is no other way besides that flag.

The only thing that would be good to add is a comment in the code that highlights the hand-differentiated or hand-modified tapenade code.

[bernardopacini]

I agree. The best solution here would be to have the team developing Tapenade update their end so that it all works properly with GCC 10+. But, I don't think this is realistic in the time frame we are working for. I am fine with either option, I have gone with the flag option in the past simply because it does not require gymnastics, as you said.

If we stick with this, the changes are good by me. But I do personally lean towards the flag option just so that we avoid possible modifications like this in the future, whenever an incompatibility arises.

[ewu63]

• The comments just before computenodalproperties_b says

  ! This originally came from tapenade, but has to extensively
  ! manually modified for it work iwthout having an enitrely new
  ! kd_tree_b.

• I forget why crossNorm had to be defined as a vector, but I will try it again this afternoon. In any case I don't think using compiler options to bypass warnings is a good idea.

[anilyil]

Sorry I did not see that comment. Based on the discussion here I thought there was not such an explanation in the code so that is fine. I agree with avoiding the compiler flag.

[ewu63]

Okay I now remember, the rank mismatch issue has to do with TAPENADE using these ARRAY functions for some reason even if the arguments are scalar. In the future maybe if we switch to a newer version of TAPENADE it would be smarter, but I had trouble getting 3.16 working. I think we should just merge this.

[bernardopacini]

Quick note: 3.16 has the same issue so that unfortunately won't fix it. Merging this sounds good to me.

[ewu63]

Quick note: 3.16 has the same issue so that unfortunately won't fix it. Merging this sounds good to me.

Oh right, because this is stuff in the ADFirstAidKit?

[bernardopacini]

Quick note: 3.16 has the same issue so that unfortunately won't fix it. Merging this sounds good to me.

Oh right, because this is stuff in the ADFirstAidKit?

Yes, exactly.

[anilyil]

Sounds good. I approved but I am also not in this list so it probably does not count. Merging is fine for me.

[ewu63]

I will merge since we have three approvals.