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

Burner ion flame #546

Closed
wants to merge 4 commits into from
Closed

Burner ion flame #546

wants to merge 4 commits into from

Conversation

BangShiuh
Copy link
Contributor

This PR is about developing IonFlow/IonFlame class.

Changes proposed in this pull request:

  • Merge class FreeFlame and AxisymmetricStagnationFlow back to StFlow. Both classes are short and can be replaced by using a flag in StFlow. This make the programming flow simpler.
  • Make IonFlow inherit from StFlow, so FreeFlame or AxisymmetricStagnationFlow can be choose, and used with IonFlow.
  • Add a new 1D simulation BurnerIonFlame. This python class can solved the profile of ions for a burner flame.

Note: this PR make FreeFlame and AxisymmetricStagnationFlow deprecated. I will add the description later if you think this is a good approach.

@codecov
Copy link

codecov bot commented Jul 10, 2018

Codecov Report

Merging #546 into master will increase coverage by <.01%.
The diff coverage is 85.24%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #546      +/-   ##
=========================================
+ Coverage    64.8%   64.8%   +<.01%     
=========================================
  Files         386     386              
  Lines       40864   40846      -18     
=========================================
- Hits        26481   26470      -11     
+ Misses      14383   14376       -7
Impacted Files Coverage Δ
include/cantera/oneD/StFlow.h 100% <ø> (ø) ⬆️
include/cantera/oneD/IonFlow.h 0% <ø> (ø) ⬆️
src/clib/ctonedim.cpp 0% <0%> (ø) ⬆️
src/oneD/Sim1D.cpp 71.06% <100%> (ø) ⬆️
src/oneD/IonFlow.cpp 73.97% <100%> (ø) ⬆️
src/oneD/refine.cpp 76.11% <100%> (ø) ⬆️
src/oneD/StFlow.cpp 85.66% <90.38%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 333d388...e1397c1. Read the comment docs.

@BangShiuh BangShiuh changed the title New ion flame Burner ion flame Jul 11, 2018
@speth
Copy link
Member

speth commented Aug 1, 2018

I was a bit ambivalent about this to start, because normally the object-oriented approach of using different derived classes to provide different functionality is a better design than a monolithic class with various options. However, in this case, there are two different dimensions in which we want to provide flexibility - one related to the boundary conditions and the continuity equation, and one for the actual governing equations being solved. While we could use some more complicated approaches using templates or multiple inheritance, I think since there are only a few distinct cases that need to be handled for the continuity equation (namely, the axisymmetric and freely-propagating flows), it makes sense to combine that option in the StFlow class and leave creating derived classes from StFlow as the method for supporting other variations in the model.

I just pushed an alternative to your first commit here (for putting the functionality of the FreeFlame and AxiStagnFlow classes into the StFlow class) to speth@edc360c. This does a couple of things that I would prefer:

  • Keep the AxiStagnFlow and FreeFlame classes for now. Deprecating them makes sense, but they can't be removed just yet.
  • Avoid adding more parameters to the constructor for StFlow. Selecting between the two options should be done using member functions. In this case, I might go with setFreeFlow() and setAxisymmetricFlow() to set the value of m_type accordingly.
  • Use a numeric constant for checks of the free vs opposed flow flag, rather than (slow) string comparisons.

I think this should provide a basis for making IonFlow derive from StFlow, and have both of these classes be usable for both freely-propagating and opposed flow flames, while keeping the current lightweight wrapper classes around in a deprecated form.

@BangShiuh
Copy link
Contributor Author

I have one question. According to your proposed commit, IonFlow inherits from FreeFlame. Do you think it will be good to change to StFlow? It is more reasonable to use StFlow and setFreeFlow or setAxisymmetricFlow from my opinion.

@speth
Copy link
Member

speth commented Aug 3, 2018

Yes, exactly. I just thought it made sense to break this up into a few distinct commits to help make it clear what's happening. So the commits in the end would be something like this:

  • Use domainType() to get distinct behavior from opposed vs freely-propagating flames (the commit I made) instead of overriding methods
  • A commit to add the new functions for setting the domain type accordingly and use these instead of the FreeFlame and AxiStagnFlow classes
  • A commit to make IonFlow inherit from StFlow
  • A commit to deprecate FreeFlame and AxiStagnFlow

@speth
Copy link
Member

speth commented Aug 10, 2018

Superseded by #548

@speth speth closed this Aug 10, 2018
@BangShiuh BangShiuh deleted the newIonFlame branch February 18, 2019 02:31
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.

2 participants