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

Remove local complexify and use shared library and scripts #82

Merged
merged 9 commits into from
Aug 4, 2023

Conversation

eirikurj
Copy link
Contributor

@eirikurj eirikurj commented Aug 1, 2023

Purpose

This PR removes the local complexify script and module and uses the shared complexify library. Docs are also updated to reflect these changes.

Expected time until merged

1 week

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 Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • 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

@eirikurj eirikurj requested a review from a team as a code owner August 1, 2023 10:43
@eirikurj eirikurj requested review from anilyil and sseraj August 1, 2023 10:43
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #82 (d0e58bf) into main (dda9b29) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   73.43%   73.50%   +0.07%     
==========================================
  Files           6        6              
  Lines         753      755       +2     
==========================================
+ Hits          553      555       +2     
  Misses        200      200              

see 2 files with indirect coverage changes

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

@eirikurj
Copy link
Contributor Author

eirikurj commented Aug 1, 2023

The Tapenade issues are to be due to a recent update/change in Tapenade behavior. Not sure why they changed this, but now they are appending *4 to an integer declaration. Compiling this code will fail with something like,

   29 |    INTEGER*4 :: branch
      |            1
Error: GNU Extension: Nonstandard type declaration INTEGER*4 at (1)

This is because we are requesting to follow a specific standard (2008 in this case) where this is not allowed. Dropping -std=f2008 would compile the code. I already encountered this as part of supporting Tapenade 3.16 for ADflow. There I opted for searching and removing these in the Tapenade generated code. A better approach would be to define a kind for these variables, and this should probably be done directly at the Tapenade level. However, I am not sure if they will support this as they only claim to support 95, with limited support for newer standards.

@sseraj
Copy link
Collaborator

sseraj commented Aug 1, 2023

I am confused why Tapenade made this breaking change and why it was seemingly backported to the 3.16 distrib tarball that we download in our Tapenade check. Ideally, we would be able to pin a static version of Tapenade, but I don't see a publicly accessible archive of previous distributions on their GitLab page. I still have the 3.16 tarball that works on my computer, so we could self-host like we do for 3.10.

@eirikurj
Copy link
Contributor Author

eirikurj commented Aug 1, 2023

Yes, I have to agree, not sure what the motivation was. They seem to generate a binary release directly from their develop branch and post online. Ideally, we would like to use the latest and know as soon as there are changes, but as you said maybe its better here to just use a fixed version of Tapenade. They have all their releases here https://gitlab.inria.fr/tapenade/tapenade/-/packages/894, so perhaps we could just download the desired release directly from there.

@sseraj
Copy link
Collaborator

sseraj commented Aug 1, 2023

They have all their releases here https://gitlab.inria.fr/tapenade/tapenade/-/packages/894, so perhaps we could just download the desired release directly from there.

Nice find! It looks like they only have releases from the develop branch, but these links are probably more stable than the link we currently use.

@eirikurj
Copy link
Contributor Author

eirikurj commented Aug 2, 2023

Created a PR for a tapenade fix here mdolab/.github#58. Hopefully this will address the issue. Will update files once merged.

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.

Looks good, but I have one minor comment on unrelated documentation

@anilyil anilyil merged commit f79222a into main Aug 4, 2023
@anilyil anilyil deleted the complexifyAsLib branch August 4, 2023 13:37
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