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

Enable additional states / equations in onedim #624

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Apr 16, 2019

Fixes # [cantera-users] Additional Equations in StFlow

Changes proposed in this pull request:

  • add nSpecies() { return m_nsp; } to StFlow.h
  • replace m_nsp = m_flow->nComponents() - c_offset_Y; by m_nsp = m_flow->nSpecies(); in boundaries1D.cpp

Proposed change enables decoupling of number of components from the number of species.

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #624 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
- Coverage   68.54%   68.53%   -0.01%     
==========================================
  Files         363      363              
  Lines       39978    39978              
==========================================
- Hits        27401    27400       -1     
- Misses      12577    12578       +1
Impacted Files Coverage Δ
src/oneD/boundaries1D.cpp 51.24% <50%> (ø) ⬆️
src/transport/GasTransport.cpp 90.77% <0%> (-0.21%) ⬇️

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 848a3bf...d3e88e2. Read the comment docs.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 16, 2019

Not sure about codecov flagging files that were not changed (GasTransport.cpp?). The push request is limited to 5 lines of code ...

PS: a functional (albeit rudimentary) example for code with added equations is posted on ischoegl/ctapp (object NewFlow defined in C++)

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

I can see how the change in Inlet1D is necessary to be able to add state variables, but I'm kind of surprised that this is sufficient. Certainly, there's a lot more work to be done to make the 1D solver more generically extensible, but this seems fine for what it does.

The coverage failures are confusing but not a problem. The test suite passing is what's important.

virtual size_t nSpecies() {
return m_nsp;
}

Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing a new method here, you could just access the number of species from Inlet1D as m_flow->phase().nSpecies().

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the simplicity of the fix surprised me also. Thanks for the suggestion: I didn't think of calling m_flow->phase(); I created the new method as neither m_thermo nor m_nsp were accessible.

I'll report back.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 16, 2019

@speth per your suggestion I eliminated the StFLow::nSpecies() method I had added, now accessing m_flow->phase().nSpecies() instead. The code compiles and the test suite passes.

@speth speth merged commit f078f53 into Cantera:master Apr 17, 2019
@ischoegl ischoegl deleted the extra-equations branch April 23, 2019 00:21
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