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

Overset update fixes #225

Merged
merged 14 commits into from
Jul 28, 2022
Merged

Overset update fixes #225

merged 14 commits into from
Jul 28, 2022

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Jun 27, 2022

Purpose

I made some fixes to the full and fast oversetUpdateMode options.

full was not working because #27 changed the cutCallback function signature but did not change it for full mode.
fast was not working in parallel because the pointer to xCen was deleted in this commit: b32cfb4.

This was leading to errors like this:

At line 1947 of file ../overset/oversetCommUtilites.F90
Fortran runtime error: Array bound mismatch for dimension 2 of array 'commpattern' (0/3)

I also fixed the adjoint routines for fast mode and updated the docs to make the drawbacks of these options clear.

Expected time until merged

One 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

I expanded the cube overset tests to test all three overset update modes. The fast and full tests fail without the fixes in this PR.

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

@sseraj sseraj requested a review from a team as a code owner June 27, 2022 23:05
@sseraj sseraj requested review from joanibal and ArshSaja June 27, 2022 23:05
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #225 (75df0bb) into main (ea9b0ed) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   41.04%   41.35%   +0.30%     
==========================================
  Files          13       13              
  Lines        3793     3794       +1     
==========================================
+ Hits         1557     1569      +12     
+ Misses       2236     2225      -11     
Impacted Files Coverage Δ
adflow/pyADflow.py 68.42% <100.00%> (+0.51%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

joanibal
joanibal previously approved these changes Jul 12, 2022
Copy link
Collaborator

@joanibal joanibal 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. I have just a few questions.

src/NKSolver/blockette.F90 Show resolved Hide resolved
!call updateOversetConnectivity_d(1_intType, sps)
if (oversetPresent) then
if (oversetUpdateMode == updateFast) then
call updateOversetConnectivity_d(1_intType, sps)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the routine always existed it just wasn't being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is related to your first question. For some reason, the fast and full updates were grouped in the residual and derivative routines. As with updateOversetConnectivity, the AD routines also only apply for fast mode. The AD routines existed and worked, but they were commented out some time ago by Ney, who was using the full update.

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.

Just left a minor comment. The rest of the PR looks good to me.

doc/options.yaml Outdated Show resolved Hide resolved
@sseraj sseraj requested review from anilyil and joanibal July 19, 2022 21:37
@anilyil anilyil merged commit b3ed666 into mdolab:main Jul 28, 2022
@sseraj sseraj deleted the overset-update branch July 28, 2022 03:36
DavidAnderegg added a commit to DavidAnderegg/adflow that referenced this pull request Aug 10, 2022
DavidAnderegg added a commit to DavidAnderegg/adflow that referenced this pull request Sep 29, 2022
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