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.

Sign up for GitHub

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

Jump to bottom

[samples] Fix heat release in ic_engine.py #820

Merged
merged 3 commits into from
Mar 23, 2020
Merged

[samples] Fix heat release in ic_engine.py #820

merged 3 commits into from
Mar 23, 2020

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Mar 11, 2020 •
edited
Loading

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

If applicable, fill in the issue number this pull request is fixing

Fixes #819

Changes proposed in this pull request

This commit fixes a bug caused by the recently introduced heat_release_rate method of SolutionArray objects (PR #810): the new method masked the pre-existing homonymous extra column.

The behavior is now corrected

In [1]: %run ic_engine.py
Heat release rate per cylinder (estimate):   35.4 kW
Expansion power per cylinder (estimate):     18.5 kW
Efficiency (estimate):                       52.3 %
CO emission (estimate):                       8.8 ppm

@ischoegl ischoegl mentioned this pull request Mar 11, 2020
Closed
@ischoegl
[samples] Fix heat release in ic_engine.py
e8c1bf2 
This commit fixes a bug caused by the recently introduced `heat_release_rate`
method of `SolutionArray` objects. The new method masked the pre-existing
homonymous `extra` column, which contained the heat release rate of the
reactor rather than heat release rate on a volumetric basis.
speth
speth reviewed Mar 20, 2020
View reviewed changes
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.

This change looks fine as a fix for the specific issue at hand. The one thing I'm wondering is whether we should prevent the use of extra columns that would be masked in this way. It seems like a bit of surprising behavior that may catch users off guard.

@ischoegl
Copy link
Member Author

The one thing I'm wondering is whether we should prevent the use of extra columns that would be masked in this way. It seems like a bit of surprising behavior that may catch users off guard.

Sounds reasonable and should be relatively straight-forward to implement. Should I attach it to this PR or open another one?

@speth
Copy link
Member

speth commented Mar 20, 2020

I'm thinking it should be fairly simple as well. If it is, feel free to add it to this PR.

@ischoegl
Copy link
Member Author

@speth ... easy enough, no surprises.

@ischoegl
[thermo] Prevent creation of pre-existing extra SolutionArray columns
1113ebb 
SolutionArray objects inherit properties from Solution objects, which will mask
newly defined 'extra' columns. This commit prevents the creation of homonymous
extra columns.
@ischoegl ischoegl force-pushed the fix-ic-engine-heat-release branch 2 times, most recently from 1b87bb6 to bb5b077 Compare March 21, 2020 00:32
@ischoegl ischoegl requested a review from speth March 21, 2020 00:51
@speth speth merged commit b449e83 into Cantera:master Mar 23, 2020
@ischoegl ischoegl deleted the fix-ic-engine-heat-release branch March 23, 2020 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@speth speth speth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Example ic_engine.py heat release rate far too high
2 participants
@ischoegl @speth