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

Revamp thermodynamic state documentation #78

Merged
merged 34 commits into from
Jul 8, 2022
Merged

Conversation

gawng
Copy link
Contributor

@gawng gawng commented Apr 18, 2022

Purpose

For computing dynamic viscosity, liquid water does not follow Sutherland's law; it follows another equation (Andrade's eqn for the interested person). I added a fix to manually set the dynamic viscosity when setting up the AeroProblem() that when you specify mu, it will override the Sutherland law calculation. Doing so avoids viscosity dependence on temperature in the self.updateViscosity() call. I decided on this fix after @awccopp said he also had a use for specifying the dynamic viscosity.

This works for all thermodynamic specification methods EXCEPT for the 'mach' + 'altitude' method.

The example aeroproblem I put in the docstring has cavitation in the evalFuncs so maybe wait until this ADflow PR is merged.

EDIT:
The new fix for water does not require overriding anything but rather, exploiting the Sutherland's law calculation. It has been documented in the code. See conversation.

Expected time until merged

Not urgent. 1-2 weeks?

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

No tests because we ended up only changing the docs.

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

@gawng gawng requested a review from a team as a code owner April 18, 2022 14:17
@gawng gawng added the enhancement New feature or request label Apr 18, 2022
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #78 (36df7ab) into main (c972540) will not change coverage.
The diff coverage is 22.41%.

@@           Coverage Diff           @@
##             main      #78   +/-   ##
=======================================
  Coverage   29.67%   29.67%           
=======================================
  Files          25       25           
  Lines        2362     2362           
=======================================
  Hits          701      701           
  Misses       1661     1661           
Impacted Files Coverage Δ
baseclasses/problems/FluidProperties.py 10.00% <ø> (ø)
baseclasses/problems/pyFieldPerformance_problem.py 16.25% <ø> (ø)
baseclasses/utils/fileIO.py 88.23% <ø> (ø)
baseclasses/problems/pyLG_problem.py 5.60% <6.25%> (ø)
baseclasses/problems/pyWeight_problem.py 10.05% <9.09%> (ø)
baseclasses/problems/pyMission_problem.py 8.13% <11.11%> (ø)
baseclasses/problems/pyAero_problem.py 18.48% <14.28%> (ø)
baseclasses/problems/ICAOAtmosphere.py 5.88% <20.00%> (ø)
baseclasses/solvers/pyAero_solver.py 19.33% <33.33%> (ø)
baseclasses/testing/pyRegTest.py 87.95% <100.00%> (ø)

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

@marcomangano
Copy link
Contributor

Why are you not implementing this for the MACH + altitude option too? Is that because the specific combination should explicitly rely on the ICAOAtmosphere model?

@gawng
Copy link
Contributor Author

gawng commented Apr 18, 2022

@marcomangano That was my logic with regards to not implementing it for that option but if you think it makes sense to be able to override it, I can fix that. I actually need to write some regression testing to make sure this code is covered (coverage just went down) so do not merge this yet.

@joanibal
Copy link
Collaborator

I'm fine with this feature in principle.
I'm a little wary that people can over define their thermodynamic state by specifying 3 state variables (like P+T+mu). But as long as we document this I'm good with it.

joanibal
joanibal previously approved these changes Apr 18, 2022
@gawng gawng marked this pull request as draft April 18, 2022 19:03
@marcomangano
Copy link
Contributor

I'm fine with this feature in principle. I'm a little wary that people can over define their thermodynamic state by specifying 3 state variables (like P+T+mu). But as long as we document this I'm good with it.

Yeah I see your concern. The point of this PR is to enable the user to override the Sutherland's law calculation so the overall thermodynamic state should still be self-consistent. Maybe we can have the code printing a message when it detects the mu input that explicitly states that Sutherland's law is ignored.

@marcomangano
Copy link
Contributor

@marcomangano That was my logic with regards to not implementing it for that option but if you think it makes sense to be able to override it, I can fix that. I actually need to write some regression testing to make sure this code is covered (coverage just went down) so do not merge this yet.

@awccopp I assume your cases with "custom" mu were still using air properties? If that is the case, we should probably have the option for mach + altitude too.

@sseraj
Copy link
Collaborator

sseraj commented Apr 21, 2022

To dos:

  • mu override in the _update to reduce code duplication (Galen)
  • Add a warning when Sutherland's law is ignored (Galen)
  • Document the steps and equations used for each input state combination and how the non-input variables are computed (Sabet)
  • Document which variables are set as inputs to ADflow (look at src/initFlow/initializeFlow.F90)
  • Clean up all mu changes (Galen)

@gawng gawng changed the title Option to fix the dynamic viscosity Revamp thermodynamic state documentation and add option to fix the dynamic viscosity Apr 25, 2022
@gawng
Copy link
Contributor Author

gawng commented May 31, 2022

@sseraj I had a look at
https://github.com/mdolab/adflow/blob/7180c03d24ed9ea1f3852861430eb4c76373d37e/src/initFlow/initializeFlow.F90#L50-L51
and it looks like viscosity is computed again via Sutherland's Law so now I'm second-guessing why we have mu being computed 2 times: once in baseclasses and then again in adflow. The dimensional viscosity in the fortran layer is used to compute the non-dimensional muInf so I think this does have an effect?

@sseraj sseraj changed the title Revamp thermodynamic state documentation and add option to fix the dynamic viscosity Revamp thermodynamic state documentation Jun 17, 2022
@gawng
Copy link
Contributor Author

gawng commented Jun 20, 2022

Codecov is failing because I changed numpy to np. The solution @sseraj and I came to was to document how to better specify mu in the AeroProblem setup. If one sets muSuthDim to the desired viscosity and sets T equal to TSuthDim, then you recover the right viscosity since Sutherland's equation will just give a unity multiplier.

With this solution, no overriding of Sutherland's law has to take place in baseclasses nor ADflow and the viscosity in the entire CFD domain can be computed properly in case there are significant temperature changes.

@joanibal
Copy link
Collaborator

Is this PR ready for review?

@sseraj
Copy link
Collaborator

sseraj commented Jun 21, 2022

Is this PR ready for review?

There is one item left for me to complete (adding docs for each input combination). I will try to complete that soon.

@joanibal
Copy link
Collaborator

No rush. I just wasn't sure because my review was requested but it is still marked as a draft PR.

@sseraj sseraj marked this pull request as ready for review June 23, 2022 20:05
@sseraj
Copy link
Collaborator

sseraj commented Jun 23, 2022

This is ready for review now @joanibal @eirikurj

@gawng gawng requested a review from joanibal July 6, 2022 13:10
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.

The docs are much clearer now, thanks for the work! Unless @eirikurj wants to take a look I will merge it soon

@marcomangano marcomangano merged commit 31470b8 into mdolab:main Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants