Fix automatic flame solving when Soret diffusion is enabled #511

bryanwweber · 2018-03-05T23:42:58Z

Changes proposed in this pull request:

Throw an exception if mixture-averaged transport is specified but Soret diffusion is enabled
Turn off Soret diffusion (if enabled) in the first solving stages of the automatic flame solver

I wasn't sure if there should be an exception thrown or if we should (silently?) force Soret to be false when mixture-averaged transport is used. Also, although the test is for a FreeFlame, this should apply to all the flames equally.

speth

Looks reasonable to me, with a few small modifications.

speth · 2018-03-06T01:34:35Z

interfaces/cython/cantera/onedim.pyx

+        # Do initial steps without Soret diffusion.
+        # Has to be done before the multicomponent check because setting
+        # the transport model to 'Mix' with soret_enabled throws an error
+        solve_soret = any(getattr(dom, 'soret_enabled', False) for dom in self.domains)


What about writing

soret_doms = [dom for dom in self.domains if getattr(dom, 'soret_enabled', False)]

then just using this list below, to spare the isinstance checks (and the if as well, for that matter)?

OK, this should work since an empty list will evaluate to False for the checks further down

speth · 2018-03-06T01:38:01Z

src/oneD/StFlow.cpp

@@ -147,6 +147,13 @@ void StFlow::setTransport(Transport& trans)
    m_trans = &trans;
    m_do_multicomponent = (m_trans->transportType() == "Multi");

+    if (!m_do_multicomponent && m_do_soret) {
+        throw CanteraError("setTransport",
+                           "Thermal diffusion (the Soret effect) is "


Extremely minor: with longer error messages, I tend to indent subsequent lines just one level, rather than lining up after the (, which gets to be a bit ridiculous once you're already 30 columns in.

speth · 2018-03-06T01:40:14Z

src/oneD/StFlow.cpp

+                           "Thermal diffusion (the Soret effect) is "
+                           "enabled, and requires using a multicomponent "
+                           "transport model.");
+    }


So if we are doing this check here, can we get rid of the one in StFlow::enableSoret? That would make it possible to toggle these flags in either order, and all that matters is that they are consistent once you call solve, which seems like an improvement to me.

I don't think we can. The check here is only for the case where Soret is enabled and you're trying to set the transport back to mixture averaged. I think if we want to be able to set the flags in either order (i.e., set m_do_soret = True when m_do_multicomponent == False or set m_do_multicomponent = False when m_do_soret == True), then we should automatically enable/disable multicomponent transport. For instance, what if the user enables Soret diffusion with the mixture averaged model and then tries to compute the diffusion coefficients? Is that affected by the Soret option?

I think it would be better to make sure the transport is always in a consistent state, either by forcing the user to manage it, or by doing it in the code. Since there are effectively only three options for consistency, it shouldn't be hard to manage automatically, if we'd prefer that. I guess I have a mild preference for making the user do it, given that I chose to write an exception rather than just manage it (also, the exception was easier to write 😄)

But I should also note that I have little experience with managing this part of the code, so I'll defer to your expertise if you think we should remove the other check.

I agree that the user should always be expected to set a valid combination of flags, with no magic that changes either the soret_enabled flag or the m_do_multicomponent flag. But in the case where the user is switching from (no soret & no multicomponent) to (soret & multicomponent) or vice versa, I think we can let them set those flags in either order, rather than forcing them to do it in a particular order. And I think that this test here is sufficient to achieve that.

I don't think there is any issue with the user doing other things besides calling the solve method -- all the methods for calculating transport properties are independent of the settings of the Sim1D object. For example, the Soret diffusion coefficients can be calculated iff a multicomponent transport object is loaded, whether or not the attached Sim1D object is set to use mixture averaged or multicomponent properties.

Just to make sure I'm understanding everything: I don't think just this test is sufficient. If the state is (no soret & no multicomponent), and the test is removed from enableSoret, the user can enable Soret and run solve, there's no check that multicomponent is enabled (unless you're suggesting I add a check in the solve method?) On the other hand, if the state is (soret & multicomponent), and the user tries to set mixture-averaged, and the test I've added here is not in place, then the state becomes (soret & no multicomponent) and the user can go and solve with no exceptions raised (until the simulation fails). Moreover, in that case (provided the test in the enableSoret function is still in place) the user cannot even turn off Soret without first turning multicomponent back on and turning off Soret first, so its a very confusing state of affairs (I realize you're not suggesting I remove this new check).

I guess I don't see a way (without having another check somewhere else) to allow the flags to be specified in either order and remain in a consistent state. Am I missing something?

Also, what happens in Sim1D when Soret diffusion coefficients are enabled but the underlying Phase is mixture-averaged? How does it get diffusion coefficients, and if they're the same as the "normal" mixture averaged coefficients, why does a simulation that runs fine for a regular mixture-averaged case fail when Soret is incorrectly enabled?

speth · 2018-03-06T01:45:28Z

interfaces/cython/cantera/test/test_onedim.py

+    def test_set_mix_with_soret(self):
+        """Test that setting mixture averaged transport with
+        Soret diffusion enabled raises an error.
+        """


Don't ask me why, but unittest doesn't get along well with docstrings. See https://travis-ci.org/Cantera/cantera/jobs/349570469#L3048. The easiest thing to do is just write this as a comment.

codecov · 2018-03-06T18:53:18Z

Codecov Report

Merging #511 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #511   +/-   ##
=======================================
  Coverage   64.78%   64.78%           
=======================================
  Files         383      383           
  Lines       40734    40734           
=======================================
  Hits        26390    26390           
  Misses      14344    14344

Impacted Files	Coverage Δ
src/oneD/StFlow.cpp	`85.89% <100%> (ø)`	⬆️
include/cantera/oneD/StFlow.h	`100% <100%> (ø)`	⬆️

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 c9b0bce...ed648d3. Read the comment docs.

speth

I still think we can get rid of the other multi/soret check, but otherwise this looks good (assuming you want to squash a few things before merging). And hooray for fixing the latest 💩 that Homebrew left us.

speth · 2018-03-07T00:18:33Z

src/oneD/StFlow.cpp

+            "Thermal diffusion (the Soret effect) is "
+            "enabled, and requires using a multicomponent "
+            "transport model."
+        );


I was assuming this would fit on fewer lines now. I also prefer the close parentheses on the same line, fwiw (I know, nitpicky).

OK I'll fix it up, and yes, I do plan to squash the commits before merging 😄

speth · 2018-03-07T00:19:26Z

interfaces/cython/cantera/test/test_onedim.py

+        self.sim.transport_model = 'Multi'
+        self.sim.soret_enabled = True
+        with self.assertRaises(ct.CanteraError):
+            self.sim.transport_model = 'Mix'


Right, so I guess my suggestion about how this should work breaks the ability to write fast tests...

Unfortunately, GitHub hid my previous comment: #511 (comment) and I didn't see these down here until just now... Can you clarify that other comment?

speth · 2018-03-10T04:08:12Z

Sorry, I was misreading the diff, and didn't realize where the new exception was being raised. The behavior I am suggesting is this:

# Currently allowed:
f.transport_model = 'Multi'
f.soret_enabled = True
f.solve(loglevel)

f.soret_enabled = False
f.transport_model = 'Mix'
f.solve(loglevel)

# These should also work:
f.soret_enabled = True
f.transport_model = 'Multi'
f.solve(loglevel)

f.transport_model = 'Mix'
f.soret_enabled = False
f.solve(loglevel)

# This should throw an exception:
f.transport_model = 'Mix'
f.soret_enabled = False
f.solve(loglevel)

I think that moving the check for m_do_soret && !m_do_multicomponent to StFlow::_finalize will give us exactly this behavior, and this is the only point where we should check for the combination of these flags.

As far as my other comment, the point is that now writing a test for this requires actually calling solve, but I guess in the case where it should generate an exception, that will still be fast. The case that is harder to write a fast test for is one that uses the auto option with the Soret effect term enabled.

bryanwweber · 2018-03-12T20:10:31Z

I tried to move the check to _finalize, but now the test test_soret_with_mix fails (in fact, it doesn't catch the exception at all and the test suite crashes out) and I don't know why. Any hints appreciated.

bryanwweber · 2018-03-12T21:17:56Z

Although, I'm realizing now that I didn't do too much to try and debug this... let me take a look with pdb and see if I can sort it out.

speth · 2018-03-12T22:21:04Z

Oh, I see what's happening. _finalize is called indirectly by Sim1D::setFixedTemperature, which means that it needs to have except +translate_exception added to its declaration in _cantera.pxd (line 751). Otherwise, the exception isn't propagated through to Python as intended.

bryanwweber · 2018-03-12T23:41:41Z

Thanks @speth. Are there any other functions where this might happen? How can I trace that?

speth · 2018-03-13T03:01:26Z

The method I used for identifying this as the cause was to run the test suite through gdb, e.g.

PYTHONPATH=build/python3 gdb --args python3 -m unittest -v cantera.test.test_onedim

Then type run, wait for the exception, and type where to get the traceback.

The Doxygen docs actually provide a route to seeing where these functions are called from: http://cantera.github.io/dev-docs/doxygen/html/classCantera_1_1Sim1D.html#a32d626626eee0bc4ade146973f6abb1c says that Sim1D::finalize (which is the only function that calls Domain1D::_finalize) is called from Sim1D::refine, Sim1D::restore, and Sim1D::setFixedTemperature. The other two functions here already have the correct declaration in _cantera.pxd.

The user can enable Soret (thermal) diffusion or multicomponent transport in either order, but attempts to solve flame problems with Soret enabled and the mixture-averaged transport approximation will result in an error

Don't use Soret diffusion for the initial solve steps, and re-enable it at the end if the user desires

If Soret diffusion and mixture-averaged transport properties are enabled, test that an exception is thrown. Also test that multicomponent diffusion and Soret diffusion can be enabled/disabled in either order. Also test that the automatic flame solver correctly disables Soret diffusion.

Homebrew switched their default Python recipe to Python 3, so install python@2 recipe. Also, directly specify which version of Python should run SCons to prevent picking up the wrong version of Python as the sys.executable.

speth reviewed Mar 6, 2018

View reviewed changes

speth reviewed Mar 7, 2018

View reviewed changes

[1D] Throw an exception if transport state is inconsistent

a03afbd

The user can enable Soret (thermal) diffusion or multicomponent transport in either order, but attempts to solve flame problems with Soret enabled and the mixture-averaged transport approximation will result in an error

bryanwweber force-pushed the fix_soret_auto branch from 0861b69 to 742bfbc Compare March 13, 2018 14:17

bryanwweber added 3 commits March 13, 2018 14:32

[1D] Check for Soret diffusion in auto flame solver

4bd5e92

Don't use Soret diffusion for the initial solve steps, and re-enable it at the end if the user desires

[CI] Fix macOS TravisCI builds

ed648d3

Homebrew switched their default Python recipe to Python 3, so install python@2 recipe. Also, directly specify which version of Python should run SCons to prevent picking up the wrong version of Python as the sys.executable.

bryanwweber force-pushed the fix_soret_auto branch from 742bfbc to ed648d3 Compare March 13, 2018 18:32

bryanwweber merged commit ed648d3 into Cantera:master Mar 13, 2018

bryanwweber deleted the fix_soret_auto branch May 31, 2018 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix automatic flame solving when Soret diffusion is enabled #511

Fix automatic flame solving when Soret diffusion is enabled #511

bryanwweber commented Mar 5, 2018

speth left a comment

speth Mar 6, 2018

bryanwweber Mar 6, 2018

speth Mar 6, 2018

bryanwweber Mar 6, 2018

speth Mar 6, 2018

bryanwweber Mar 6, 2018

bryanwweber Mar 6, 2018

speth Mar 7, 2018

bryanwweber Mar 7, 2018

speth Mar 6, 2018

bryanwweber Mar 6, 2018

codecov bot commented Mar 6, 2018 •

edited

Loading

speth left a comment

speth Mar 7, 2018

bryanwweber Mar 9, 2018

speth Mar 7, 2018

bryanwweber Mar 9, 2018

speth commented Mar 10, 2018

bryanwweber commented Mar 12, 2018 •

edited

Loading

bryanwweber commented Mar 12, 2018

speth commented Mar 12, 2018

bryanwweber commented Mar 12, 2018

speth commented Mar 13, 2018 •

edited

Loading

Fix automatic flame solving when Soret diffusion is enabled #511

Fix automatic flame solving when Soret diffusion is enabled #511

Conversation

bryanwweber commented Mar 5, 2018

speth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speth commented Mar 10, 2018

bryanwweber commented Mar 12, 2018 • edited Loading

bryanwweber commented Mar 12, 2018

speth commented Mar 12, 2018

bryanwweber commented Mar 12, 2018

speth commented Mar 13, 2018 • edited Loading

codecov bot commented Mar 6, 2018 •

edited

Loading

bryanwweber commented Mar 12, 2018 •

edited

Loading

speth commented Mar 13, 2018 •

edited

Loading