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

Turning off extraneous aeroProblem print outs from MPhys wrapper #307

Merged
merged 27 commits into from
Nov 4, 2023

Conversation

lamkina
Copy link
Contributor

@lamkina lamkina commented Aug 19, 2023

Purpose

This PR adds arguments to setAeroproblem and _setAeroProblemData in pyADflow to turn off printing of angle of attach and BC warnings. To turn off boundary condition warning messages, a new logical argument was added to setBCDataFineGrid subroutine in the BCData module. The usage of the setBCDataFineGrid in other subroutines was updated to maintain current functionality.

Now we turn off the boundary conition printing with fortran module variables.

The docstrings for setAeroproblem and _setAeroProblemData were also updated.
The docstrings were set back to their previous state now that we use a module variable.

An example of the printouts that happen everytime setAeroproblem is called is shown below. This happens during the coupled adjoint and primal solve. The angle of attack and boundary condition warnings are printed several times because of the extra calls required in MPhys, even though sometimes we are not actually switching the aeroproblem.

============
TOC.coupling
============
-> Alpha... 0.000000 
-> Alpha... 0.000000 
LN: LNBGS 0 ; 2006628.67 1
-> Alpha... 0.000000 
Solving linear in mphys_adflow
Solving ADjoint Transpose with PETSc...
   0 KSP Residual norm 9.1932257536E+03
  10 KSP Residual norm 9.1898236124E+03
  20 KSP Residual norm 9.1880786747E+03
  30 KSP Residual norm 9.1559423727E+03
  40 KSP Residual norm 9.0338778426E+03
  50 KSP Residual norm 8.3380256715E+03
  60 KSP Residual norm 6.1930959481E+03
  70 KSP Residual norm 3.5505594710E+03
  80 KSP Residual norm 1.4910352070E+03
  90 KSP Residual norm 6.3038686621E+02
 100 KSP Residual norm 2.1666819396E+02
 110 KSP Residual norm 7.8767925855E+01
 120 KSP Residual norm 2.2629509372E+01
 130 KSP Residual norm 6.3923943385E+00
 140 KSP Residual norm 1.6993385453E+00
 150 KSP Residual norm 4.0424036578E-01
 160 KSP Residual norm 1.3088391770E-01
Solving ADjoint Transpose with PETSc time (s) =    37.11
 Norm of error = 8.1977E-02    Iterations =  164
 ------------------------------------------------
 PETSc solver converged after   164 iterations.
 ------------------------------------------------
#
#*==================== !!! Warning !!! ======================
# Subsonic inflow regions present for which the turbulence
# quantities are not or insufficiently prescribed.
# Using free stream values instead.
#*===========================================================
#
#
#*==================== !!! Warning !!! ======================
# Subsonic inflow regions present for which the turbulence
# quantities are not or insufficiently prescribed.
# Using free stream values instead.
#*===========================================================
#
#
#*==================== !!! Warning !!! ======================
# Subsonic inflow regions present for which the turbulence
# quantities are not or insufficiently prescribed.
# Using free stream values instead.
#*===========================================================
#
#
#*==================== !!! Warning !!! ======================
# Subsonic inflow regions present for which the turbulence
# quantities are not or insufficiently prescribed.
# Using free stream values instead.
#*===========================================================
#
-> Alpha... 0.000000 
-> Alpha... 0.000000 
-> Alpha... 0.000000 
LN: LNBGS 1 ; 8.99476042 4.48252363e-06
-> Alpha... 0.000000 
Solving linear in mphys_adflow
Solving ADjoint Transpose with PETSc...
   0 KSP Residual norm 9.4033593773E+01
  10 KSP Residual norm 9.3851727433E+01
  20 KSP Residual norm 9.3812444502E+01
  30 KSP Residual norm 9.3662893414E+01
  40 KSP Residual norm 9.2378872212E+01
  50 KSP Residual norm 8.8436313339E+01
  60 KSP Residual norm 5.1510695403E+01
  70 KSP Residual norm 1.7465079446E+01
  80 KSP Residual norm 7.2535328261E+00
  90 KSP Residual norm 3.7911121457E+00
 100 KSP Residual norm 1.1015529443E+00
 110 KSP Residual norm 2.6571294463E-01
 120 KSP Residual norm 7.1421136189E-02
 130 KSP Residual norm 1.7754931595E-02
 140 KSP Residual norm 5.2134242805E-03
 150 KSP Residual norm 2.0794183305E-03
Solving ADjoint Transpose with PETSc time (s) =    35.69
 Norm of error = 8.7121E-04    Iterations =  158
 ------------------------------------------------
 PETSc solver converged after   158 iterations.

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

Only visual testing. I ran runscripts with and without the change to ensure the printing flags were working correctly.

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

@lamkina lamkina requested review from A-CGray and ArshSaja August 19, 2023 22:00
@lamkina lamkina requested a review from a team as a code owner August 19, 2023 22:00
@lamkina lamkina requested review from sseraj and removed request for sseraj August 19, 2023 22:00
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #307 (cc8f7e6) into main (2f76d81) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
- Coverage   41.94%   41.87%   -0.08%     
==========================================
  Files          13       13              
  Lines        4005     4017      +12     
==========================================
+ Hits         1680     1682       +2     
- Misses       2325     2335      +10     
Files Coverage Δ
adflow/pyADflow.py 69.02% <ø> (+0.08%) ⬆️
adflow/mphys/mphys_adflow.py 0.00% <0.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

A-CGray
A-CGray previously approved these changes Aug 22, 2023
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

LGTM, could you just update the description by adding a printout example for reference?

@lamkina
Copy link
Contributor Author

lamkina commented Aug 24, 2023

I just updated the description. I realized I turned off printing in solve_linear in the ADflowSolver component. Once I switch that back on this should be ready for another review.

@lamkina lamkina requested a review from A-CGray August 24, 2023 13:53
marcomangano
marcomangano previously approved these changes Aug 24, 2023
adflow/pyADflow.py Outdated Show resolved Hide resolved
adflow/pyADflow.py Outdated Show resolved Hide resolved
@A-CGray
Copy link
Member

A-CGray commented Sep 6, 2023

Bump on this

@eirikurj
Copy link
Contributor

eirikurj commented Sep 8, 2023

@lamkina I just posted a response today, it should already be there #307 (comment).

@lamkina
Copy link
Contributor Author

lamkina commented Sep 8, 2023

@lamkina I just posted a response today, it should already be there #307 (comment).

Sorry I missed the notification. Thanks!

@lamkina
Copy link
Contributor Author

lamkina commented Sep 14, 2023

@eirikurj @A-CGray @marcomangano I updated this PR based on the feedback to switch to option and module variables to control the printing. I removed all the arguments and replaced them with a module variable in the fortran layer that we only call in MPhys called printbcwarnings. I did not expose this as a global ADflow option because it's an MPhys only use case that normal MACH users shouldn't touch.

I also added some new logic to determine when to print the BC warnings and alpha updates in the mphys layer in setAeroProblem. In a nutshell, it checks if the AP changed. If the AP changes we always print the information. If the AP didn't change, it then checks if we are doing a new solver iteration. If the iteration is different for the same AP, then it prints the information (i.e single point opt with the same aeroproblem).

A-CGray
A-CGray previously approved these changes Sep 25, 2023
Copy link
Member

@A-CGray A-CGray left a comment

Choose a reason for hiding this comment

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

I ran this myself and it does what it's supposed to do

@lamkina lamkina mentioned this pull request Sep 26, 2023
@A-CGray
Copy link
Member

A-CGray commented Sep 26, 2023

@marcomangano @ArshSaja this is good to go

@A-CGray
Copy link
Member

A-CGray commented Sep 28, 2023

@marcomangano @ArshSaja bump

@lamkina
Copy link
Contributor Author

lamkina commented Oct 2, 2023

@ArshSaja @marcomangano bump again

@sseraj
Copy link
Collaborator

sseraj commented Oct 2, 2023

@eirikurj needs to approve because he requested changes

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Added few minor comments.

adflow/mphys/mphys_adflow.py Outdated Show resolved Hide resolved
adflow/pyADflow.py Outdated Show resolved Hide resolved
adflow/pyADflow.py Outdated Show resolved Hide resolved
A-CGray
A-CGray previously approved these changes Nov 3, 2023
Copy link
Member

@A-CGray A-CGray left a comment

Choose a reason for hiding this comment

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

If @eirikurj 's happy with this PR then I am too

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the effort on this. BTW, instead of requesting changes, I just adjusted the case on the Fortran variable to be consistent.

@A-CGray
Copy link
Member

A-CGray commented Nov 4, 2023

I approve but don't have permission to merge

@lamkina lamkina merged commit 409b986 into mdolab:main Nov 4, 2023
15 of 16 checks passed
@lamkina lamkina deleted the mphys_fix branch November 4, 2023 16:41
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