Closed ewu63 closed 2 years ago
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.
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.
@bernardopacini or @anily could you take a look at this PR?
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?
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?
Would that flag hide any other inconsistencies when adding new code?
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.
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.
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.
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.
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.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.
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.
Quick note: 3.16 has the same issue so that unfortunately won't fix it. Merging this sounds good to me.
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?
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.
Sounds good. I approved but I am also not in this list so it probably does not count. Merging is fine for me.
I will merge since we have three approvals.
Purpose
I fixed a few things in the source code such that it can be compiled by gcc 11.
outputReverse
that I believe is unused, since the same code exists inkd_tree
which is specifically hand-differentiated.Type of change
Testing
Both real and complex tests now pass.
Checklist
flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted