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 AD code structure #62

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Update AD code structure #62

merged 4 commits into from
Nov 22, 2021

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Nov 18, 2021

Purpose

I updated the entire AD code structure:

  • created a new adjoint directory where AD-related code is located in
  • updated Tapenade build process by creating a temporary directory for pre-processed files, such that they are no longer located in the original directory. This fixes some weird <something>.F90_b.f90 filename issue with 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

This PR just changes the AD build process. Existing tests should 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

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

codecov bot commented Nov 18, 2021

Codecov Report

Merging #62 (35b5b2b) into master (055874b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

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 055874b...35b5b2b. Read the comment docs.

@bernardopacini
Copy link
Collaborator

I left two broader comments about the PR. I think the changes are fine, but since we are refactoring things now it may be worth making these small changes too while we are here.

Since I worked with you (@nwu63) on this, we should have at least one more person who knows about Tapenade review this.

anilyil
anilyil previously approved these changes Nov 20, 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.

The changes look good, but then again, I am no expert on any of the files that have changes. My only broad comment would be to be careful about a possible parallel make. I am not sure if tapenade makefiles even do that, but just something that can cause race conditions with the addition and removal of temp files. If what I am saying is completely unrelated, then this PR is probably good to go for me.

@bernardopacini
Copy link
Collaborator

bernardopacini commented Nov 20, 2021

The changes look good, but then again, I am no expert on any of the files that have changes. My only broad comment would be to be careful about a possible parallel make. I am not sure if tapenade makefiles even do that, but just something that can cause race conditions with the addition and removal of temp files. If what I am saying is completely unrelated, then this PR is probably good to go for me.

As far as I know, Tapenade does not support that kind of parallelism and we do not use it anywhere even if it does. I don't think we have to worry about this, Tapenade handles IO itself and our preprocessing is serial in the makefile. Tapenade is realistically pretty quick so I think that we should avoid parallelism in the future even if it does exist -- not worth the extra effort. If Tapenade is parallel within the executable that's fine, it shouldn't change anything for our makefile.

@anilyil
Copy link
Contributor

anilyil commented Nov 21, 2021

Okay, then it looks good on my end. We need 2 approvals to merge I think.

@bernardopacini bernardopacini self-requested a review November 22, 2021 14:20
bernardopacini
bernardopacini previously approved these changes Nov 22, 2021
@ewu63 ewu63 dismissed stale reviews from bernardopacini and anilyil via 5164342 November 22, 2021 14:45
@ewu63 ewu63 merged commit 5656b04 into master Nov 22, 2021
@ewu63 ewu63 deleted the update-AD-code-structure branch November 22, 2021 15:00
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.

3 participants