Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tapenade 3.16 #63

Merged
merged 12 commits into from
Feb 8, 2022
Merged

Update tapenade 3.16 #63

merged 12 commits into from
Feb 8, 2022

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Nov 22, 2021

Purpose

This time I actually updated the Tapenade-generated source code to 3.16.

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

Existing tests all pass. I kept a tiny diff in one AD'd file to make sure our CI checks are working as expected.

@ewu63 ewu63 requested a review from a team as a code owner November 22, 2021 15:31
@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #63 (e36cd57) into master (4a8a6ed) will decrease coverage by 0.93%.
The diff coverage is n/a.

❗ Current head e36cd57 differs from pull request most recent head dd5d33a. Consider uploading reports for the commit dd5d33a to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   45.72%   44.78%   -0.94%     
==========================================
  Files           6        6              
  Lines         748      748              
==========================================
- Hits          342      335       -7     
- Misses        406      413       +7     
Impacted Files Coverage Δ
idwarp/UnstructuredMesh_C.py 46.15% <0.00%> (-53.85%) ⬇️

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 4a8a6ed...dd5d33a. Read the comment docs.

@eirikurj
Copy link
Contributor

Overall, the changes look fine. I am assuming you will update the yml file later to test with 3.16 later?

@eirikurj
Copy link
Contributor

eirikurj commented Nov 22, 2021

@nwu63 I guess you beat me to it. However, why did the tapende test not fail before?

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 22, 2021

I think when I removed those lines in commit 5409a91, this prevented tapenade from generating any new files. Therefore, git did not catch anything as it was comparing nothing to the existing files. If we really want to do this properly, we will need to also delete all existing Fortran files in those directories before running Tapenade. The current check assumes that the same filenames are being touched by Tapenade each time.

@ewu63 ewu63 requested a review from eirikurj November 29, 2021 03:58
Copy link
Contributor

@anilyil anilyil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. My only minor comment: looks like we had the actual build of tapenade printed in the headers after the version, i.e. Tapenade 3.10 (r5363) but the new version just has version and master. Is it possible to get a more informative thing printed there instead of master?

Might be a bit easier to match this run in the future, but I may be completely missing the point as well. Is the version number enough to fully define what state of tapenade we used?

@ewu63
Copy link
Collaborator Author

ewu63 commented Nov 30, 2021

I don't think there's a way to change this because we're just using compiled binaries released by them. However, I think in addition to the version, the date there refer to the day that the binaries are built, so together with the version we should be able to uniquely identify which version of tapenade was used to generate these.

@anilyil anilyil self-requested a review December 1, 2021 01:39
anilyil
anilyil previously approved these changes Dec 1, 2021
Copy link
Contributor

@anilyil anilyil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then this is good to go for me.

@ewu63
Copy link
Collaborator Author

ewu63 commented Jan 25, 2022

I'm not sure if this is waiting on anyone. @eirikurj can we merge this in?

@sseraj sseraj requested review from lamkina and removed request for SichengHe and Xiaosong2105 February 4, 2022 16:33
lamkina
lamkina previously approved these changes Feb 7, 2022
Copy link

@lamkina lamkina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what the autoEdit changes do?

@ewu63 ewu63 dismissed stale reviews from lamkina and anilyil via dd5d33a February 8, 2022 04:13
@ewu63
Copy link
Collaborator Author

ewu63 commented Feb 8, 2022

Can you clarify what the autoEdit changes do?

I added a few files so that they are not copied over to the actual output directories, meaning they will not be compiled or tracked by git. These files are generated as part of Tapenade (I'm sure there's a way to set it up such that they are not generated, but I didn't try this), but the AD'd code are not needed. The ignore list makes sure they are not copied over.

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thanks

@ewu63 ewu63 merged commit 71640df into master Feb 8, 2022
@ewu63 ewu63 deleted the update-tapenade-3.16 branch February 8, 2022 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants