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

Free stream change correction improvements #264

Merged
merged 22 commits into from
May 23, 2023

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Feb 21, 2023

Purpose

This PR adds a new way to compute a free stream correction update.

When the free stream conditions are modified for a given aeroProblem compared to the last time it was run, ADflow computes a simple correction to the states that adds the offset from the change to each cell's state. The goal here is to perturb the states enough so that the residual increases throughout the volume. If this correction is not performed, the residuals only increase near the cells that are 2 layers within the farfield BCs. As a result, the Newton-based solvers struggle to get over these transients because the residuals initially go up as the changes are propagated throughout the domain. This offset also aims to put the volume cells in a better position for convergence. The cells that have a state that is similar to the farfield conditions will be very close to their final state with the correction. This behavior is controlled with the option infChangeCorrection.

In this PR, I add a new method to perform this correction update. In my formulation, instead of adding the delta between the new and the old free stream conditions, I actually rotate the velocity vectors at each cell by how much the free stream velocity has rotated. This change results in a slightly better restart when the inflow angle is changed. I do not need to keep track of alpha and beta because the update is based on the directions of the old and new free stream velocities. The rotation is performed on the plane that is orthogonal to these vectors. On top of the rotation, I also add the changes to the density and energy throughout like the original method because these changes can become important with changes to altitude and Mach number.

There are now 3 options that control the correction update:

  • "infChangeCorrection": [bool, True], this option enables or disables this correction
  • "infChangeCorrectionTol": [float, 1e-12], this tolerance determines when we may not want to run the correction update. If the norm of the change in the state is less than this tolerance, we dont run the update. This is useful when the state does not change, so we avoid messing stuff up by running a correction. For the offset method this is not too problematic, but the rotation needs to compute an orthogonal vector, and if the two free stream vectors are parallel, it can't and would get a nan. This tolerance prevents this from happening. The tolerance was a hard coded parameter before this PR so the PR maintains the old behavior exactly.
  • "infChangeCorrectionType": [str, ["offset", "rotate"]], pretty self explanatory; offset does the original method, rotate rotates the velocities and offsets the density and energy.

Expected time until merged

1-2 weeks. I want to use this feature a bit to see if it works. I also will add a test for this.

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

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #264 (743ed5f) into main (b97914b) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
- Coverage   40.87%   40.86%   -0.02%     
==========================================
  Files          13       13              
  Lines        3892     3901       +9     
==========================================
+ Hits         1591     1594       +3     
- Misses       2301     2307       +6     
Impacted Files Coverage Δ
adflow/pyADflow.py 66.71% <100.00%> (+0.02%) ⬆️

... and 5 files with indirect coverage changes

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

@anilyil anilyil marked this pull request as ready for review March 27, 2023 07:33
@anilyil anilyil requested a review from a team as a code owner March 27, 2023 07:33
@anilyil
Copy link
Contributor Author

anilyil commented May 12, 2023

This is ready to go because it seems like my last fix fixed the issues with the tests.

Copy link
Contributor

@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.

A few comments to discuss before I approve this PR:

  1. Should we abstract away the rotation matrix as a separate routine? This would let us use this as a general utility in the future.
  2. Does it make sense to add a test for the case where the change in the states is below the tolerance? I think we might want to test that capability.

@anilyil
Copy link
Contributor Author

anilyil commented May 17, 2023

I addressed both of the changes, @lamkina

lamkina
lamkina previously approved these changes May 17, 2023
Copy link
Contributor

@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.

I think this is ready to go.

@ArshSaja
Copy link
Contributor

It looks good to me, I have a question on the infChangeCorrectionTol. In which cases this tolerance can be set to a higher value? I suppose in all the cases we would set this tolerance to be a lower value, smth like 1e-12.

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.

I only checked the docs

doc/options.yaml Outdated Show resolved Hide resolved
doc/options.yaml Outdated Show resolved Hide resolved
@anilyil
Copy link
Contributor Author

anilyil commented May 22, 2023

@ArshSaja, I added a bit more explanation in the options docs for how the tolerance should be picked.

@anilyil anilyil requested a review from lamkina May 22, 2023 19:26
@anilyil anilyil requested a review from sseraj May 23, 2023 05:32
@lamkina lamkina merged commit 994d8be into mdolab:main May 23, 2023
@anilyil anilyil deleted the inf_change_correction_mods branch May 23, 2023 13:58
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.

4 participants