Update ic_engine example using new cantera capabilities

[ischoegl]

Please fill in the issue number this pull request is fixing:

Fixes #665

Changes proposed in this pull request:

• Eliminate if-else blocks that switch valves and replace by timed valves
• Use limited advance steps for adaptive refinement
• Logic for refinement strategy is no longer necessary
• Streamline use of solution array

Excluding plots, simulation time decreases from 16.5s to 5.4s. Results are unaffected.

[bryanwweber]

Thanks @ischoegl! This looks good to me overall.

Similar to your other PR, we need some way to mark that this will work for the development version only. I think the best solution for the examples is for the website repo to check out the 2.4 branch of the Cantera repository during the CI build process, to make sure we get the examples that will be compatible with 2.4. Do you feel comfortable with that change, or do you want me to do it? I should be just one line.

[bryanwweber]

One other comment, again forgot to add it in the review 🙄 I'm sure you know that gri30 is not intended to be used for propane chemistry. I think it's used in this example because it's distributed with Cantera and is relatively small (=fast), but I don't want to encourage people to use gri30 for problems with propane. Can you try this out with the Reitz dodecane mechanism or another mechanism in data/inputs?

[ischoegl]

Thanks @ischoegl! This looks good to me overall.

Similar to your other PR, we need some way to mark that this will work for the development version only. I think the best solution for the examples is for the website repo to check out the 2.4 branch of the Cantera repository during the CI build process, to make sure we get the examples that will be compatible with 2.4. Do you feel comfortable with that change, or do you want me to do it? I should be just one line.

Not sure how CI differentiates (I had always assumed that 2.4 didn't pull from master). I'd be interested, so could you provide some pointers?

One other comment, again forgot to add it in the review roll_eyes I'm sure you know that gri30 is not intended to be used for propane chemistry. I think it's used in this example because it's distributed with Cantera and is relatively small (=fast), but I don't want to encourage people to use gri30 for problems with propane. Can you try this out with the Reitz dodecane mechanism or another mechanism in data/inputs?

Yes, I am keenly aware of that, but didn't want to change without prior discussion. Likely, the compression ratio will have to change; at least the simulations are more efficient now. It won't take long to test, and will push a revision once I see some decent results (hopefully we'll get a more reasonable efficiency also).

[bryanwweber]

In the Travis config for the website, it clones this repo. All you have to do is add a git checkout 2.4 right below the clone. Let me know if any jargon needs defining 😊

[ischoegl]

In the Travis config for the website, it clones this repo. All you have to do is add a git checkout 2.4 right below the clone. Let me know if any jargon needs defining blush

Not familiar with it, but it's probably obvious. Just haven't poked around there yet.

[ischoegl]

As an aside, there appear a bunch of nan's in the entropy calculations (as evidenced by the intermittent lines in the T-s plot). The root cause clearly goes beyond this PR, but I'm curious whether this is a known issue? (there are some negative concentrations)

[ischoegl]

In the Travis config for the website, it clones this repo. All you have to do is add a git checkout 2.4 right below the clone. Let me know if any jargon needs defining blush

Not familiar with it, but it's probably obvious. Just haven't poked around there yet.

@bryanwweber ... in .travis.yml:

before_install:
  - git clone --depth=1 https://github.com/Cantera/cantera ../cantera  --branch 2.4
  - git clone --depth=1 https://github.com/Cantera/cantera-jupyter ../cantera-jupyter

I assume website updates involve PR's from forked repos?

[bryanwweber]

@ischoegl No, the website will only be updated when new changes are merged to the master branch of the main fork. Also, it looks like the 2.4 branch is already checked out, so there shouldn't be anything that you need to do!

I'm not sure about the nan calculations here, could the problem be the mechanism?

[ischoegl]

@bryanwweber ... ok, I did look into Reitz, but realized that the mechanism is not compatible with IdealGasReactor. Switching to Reactor, I got things to run after cranking up tolerances (rtol=1.e-12, atol=1.e-16), but suffered erratic core dumps when exiting out of ipython (corrupted double-linked list or corrupted size vs. prev_size that I believe stem from the core ... i.e. this goes beyond this PR).

As there aren't any good alternatives in terms of gas mechanisms, I opted for a more detailed disclaimer in the docstring:

"""
Simulation of a (gaseous) Diesel-type internal combustion engine.

Note that this example serves for illustration purposes only. While the
approximation of an IC engine as a simple reactor network has inherent
limitations by itself, additional considerations are:
1. The reaction mechanism GRI3.0 was never intended for use with pure propane
   (it was developed for natural gas mixtures), but is used here as it allows 
   for fast simulations, i.e. predictive capabilities are limited.
2. Simulations require an unrealistically high compression ratio, i.e.
   resulting efficiency estimates are also not realistic.
"""

[ischoegl]

@ischoegl No, the website will only be updated when new changes are merged to the master branch of the main fork. Also, it looks like the 2.4 branch is already checked out, so there shouldn't be anything that you need to do!

Thanks. FYI: it was me who added the --branch 2.4 in my previous post. I guess it needs to go into .travis.yml right there?

[bryanwweber]

Yes, that should do the trick. You might also consider adding the --single-branch switch to help speed up the clone a little more.

[ischoegl]

Yes, that should do the trick. You might also consider adding the --single-branch switch to help speed up the clone a little more.

PR is already issued (I'll let CI run through before --single-branch). See Cantera/cantera-website#86

[speth]

A couple of comments from just reading the discussion:

1. You can use the Reitz mechanism as an ideal gas phase - the instantiation would just be
   gas = ct.Solution('nDodecane_Reitz.cti', 'nDodecane_IG'), corresponding to the second phase declaration in the input file.
2. You will get NaNs for entropy any time you have negative mass/mole fractions. The main time that this is encountered is when you're directly using the Solution object in the state that is left in by CVODES (which must be allowed to contain negative numbers), rather than the "corrected" state that you will get if you access it as reactor.thermo.

[ischoegl]

@speth ... thank you for the suggestions!

1. You can use the Reitz mechanism as an ideal gas phase - the instantiation would just be
   gas = ct.Solution('nDodecane_Reitz.cti', 'nDodecane_IG'), corresponding to the second phase declaration in the input file.

This works nicely! Also, no more core dumps ...

2. You will get NaNs for entropy any time you have negative mass/mole fractions. The main time that this is encountered is when you're directly using the Solution object in the state that is left in by CVODES (which must be allowed to contain negative numbers), rather than the "corrected" state that you will get if you access it as reactor.thermo.

Thanks for this hint ... however, the script does grab from Reactor.thermo.state when saving states. A quick check for the SolutionArray at the end of the simulation yields the following (which imho is not consistent).

In [6]: np.isnan(states.s).any()
Out[6]: True

In [7]: np.isnan(states.h).any()
Out[7]: False

[speth]

Sorry, my mistake. The state setting done implicitly by Reactor.thermo does not clean up the mass fraction array, and using the state attribute will also continue to use the methods for setting the "unnormalized" mass fractions.

I'm not sure why it's inconsistent for the entropy to end up as NaN while the the enthalpy does not -- the equation for the entropy requires (effectively) taking log(Y_k), which is undefined for negative numbers while the equation for the enthalpy has no similar problems.

However, I did notice that we have a workaround for this problem implemented in other methods, such as IdealGasPhase::getPartialMolarEntropies, and I don't think there's any reason not to do the same for enthalpy_mole (and implicitly, enthalpy_mass), which I've created a PR for now (#686).

[speth]

Generally looks good to me. Here are a couple of minor suggestions.