Add option for mass gradient-based fluxes #1668

g3bk47 · 2024-02-16T20:05:20Z

Changes proposed in this pull request

Add a new option to compute species diffusive fluxes with respect to their mass fraction gradients for 1D flames. See Cantera/enhancements#195 for extended discussion. All changes are fully backward compatible.

After some brainstoming, I settled on flux_gradient_basis as the new property's name, i.e. using mole or mass fraction gradients for computing species fluxes. If you have a better idea, feel free to let me know.
I also changed the API to use the established basis enum. Performance-wise, this should not have an impact, since the comparison m_fluxGradientBasis == ThermoBasis::molar is a simple hard-coded comparison against zero.

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

speth · 2024-03-09T18:04:32Z

include/cantera/oneD/StFlow.h

+    //! Compute species diffusive fluxes with respect to
+    //! their mass fraction gradients (fluxGradientBasis = ThermoBasis::mass)
+    //! or mole fraction gradients (fluxGradientBasis = ThermoBasis::molar, default)
+    //! when using the mixture-averaged transport model
+    void fluxGradientBasis(ThermoBasis fluxGradientBasis);
+    ThermoBasis fluxGradientBasis() const {
+        return m_fluxGradientBasis;
+    }


Each of these functions needs a docstring so it will be handled correctly by Doxygen. You can use @copydoc or @see to avoid having to repeat the content.

For consistency, the setter should be setFluxGradientBasis rather than overloading the same name as the getter.

speth · 2024-03-09T18:05:25Z

interfaces/cython/cantera/_onedim.pyx

+        Get/Set whether or not species diffusive fluxes are computed with
+        respect to their mass fraction gradients ('mass')
+        or mole fraction gradients ('molar', default) when
+        using the mixture-avaraged transport model.


Suggested change

using the mixture-avaraged transport model.

using the mixture-averaged transport model.

speth · 2024-03-09T18:06:08Z

interfaces/cython/cantera/onedim.py

+        Get/Set whether or not species diffusive fluxes are computed with
+        respect to their mass fraction gradients ('mass')
+        or mole fraction gradients ('molar', default) when
+        using the mixture-avaraged transport model.


Suggested change

using the mixture-avaraged transport model.

using the mixture-averaged transport model.

speth · 2024-03-09T18:08:32Z

interfaces/cython/cantera/onedim.py

+    def flux_gradient_basis(self, basis):
+        if basis == 'molar' or basis == 'mass':
+            self.flame.flux_gradient_basis = basis
+        else:
+            raise ValueError("Valid choices are 'mass' or 'molar'."
+                             " Got {!r}.".format(basis))


Checking the value of the flag here isn't really necessary -- the _FlowBase.flux_gradient_basis method can handle the error.

speth · 2024-03-09T18:12:16Z

src/oneD/StFlow.cpp

+        writelog("Warning: setting fluxGradientBasis only affects"
+                " the mixture-averaged diffusion model.");


I'd suggest using warn_user here, rather than the less-specific writelog.

speth · 2024-03-09T18:14:25Z

src/oneD/StFlow.cpp

            for (size_t k=0; k < m_nsp; k++) {
-                m_diff[k+j*m_nsp] *= m_wt[k] * rho / wtm;
+                if (m_fluxGradientBasis == ThermoBasis::molar) {
+                    m_diff[k+j*m_nsp] *= m_wt[k] * rho / wtm;
+                } else {
+                    m_diff[k+j*m_nsp] *= rho;
+                }
            }


At least for this inner loop, I think it's worth inverting the structure to check the m_fluxGradientBasis before the loop.

speth · 2024-03-09T18:14:51Z

src/oneD/StFlow.cpp

+                if (m_fluxGradientBasis == ThermoBasis::molar) {
+                    m_flux(k,j) *= (X(x,k,j) - X(x,k,j+1))/dz;
+                } else {
+                    m_flux(k,j) *= (Y(x,k,j) - Y(x,k,j+1))/dz;
+                }


Likewise in this case.

speth · 2024-03-09T18:15:34Z

src/oneD/StFlow.cpp

@@ -780,6 +804,8 @@ AnyMap StFlow::getMeta() const

    state["Soret-enabled"] = m_do_soret;

+    state["Flux-Gradient-Basis"] = static_cast<long int>(m_fluxGradientBasis);


Our naming convention for YAML fields is lower case except for proper names, so this should be flux-gradient-basis.

speth · 2024-03-09T18:18:47Z

test/python/test_onedim.py

+    def test_flux_gradient_mass(self):
+        self.create_sim(ct.one_atm, 300, 'H2:1.1, O2:1, AR:5.3')
+        self.sim.transport_model = 'mixture-averaged'
+        self.sim.set_refine_criteria(ratio=3.0, slope=0.08, curve=0.12)
+        self.assertTrue(self.sim.flux_gradient_basis == 'molar')
+        self.sim.solve(loglevel=0, auto=True)
+        sl_mole = self.sim.velocity[0]
+
+        self.sim.flux_gradient_basis = 'mass'
+        self.assertTrue(self.sim.flux_gradient_basis == 'mass')
+        self.sim.solve(loglevel=0)
+        sl_mass = self.sim.velocity[0]
+        # flame speeds should not be exactly equal
+        self.assertNotEqual(sl_mole, sl_mass)
+        # but they should be close
+        self.assertTrue(np.abs(sl_mole-sl_mass)/sl_mole < 0.1)
+


We're (slowly) transitioning to pytest style asserts, so this should be written as:

Suggested change

def test_flux_gradient_mass(self):

self.create_sim(ct.one_atm, 300, 'H2:1.1, O2:1, AR:5.3')

self.sim.transport_model = 'mixture-averaged'

self.sim.set_refine_criteria(ratio=3.0, slope=0.08, curve=0.12)

self.assertTrue(self.sim.flux_gradient_basis == 'molar')

self.sim.solve(loglevel=0, auto=True)

sl_mole = self.sim.velocity[0]

self.sim.flux_gradient_basis = 'mass'

self.assertTrue(self.sim.flux_gradient_basis == 'mass')

self.sim.solve(loglevel=0)

sl_mass = self.sim.velocity[0]

# flame speeds should not be exactly equal

self.assertNotEqual(sl_mole, sl_mass)

# but they should be close

self.assertTrue(np.abs(sl_mole-sl_mass)/sl_mole < 0.1)

def test_flux_gradient_mass(self):

self.create_sim(ct.one_atm, 300, 'H2:1.1, O2:1, AR:5.3')

self.sim.transport_model = 'mixture-averaged'

self.sim.set_refine_criteria(ratio=3.0, slope=0.08, curve=0.12)

assert self.sim.flux_gradient_basis == 'molar'

self.sim.solve(loglevel=0, auto=True)

sl_mole = self.sim.velocity[0]

self.sim.flux_gradient_basis = 'mass'

assert self.sim.flux_gradient_basis == 'mass'

self.sim.solve(loglevel=0)

sl_mass = self.sim.velocity[0]

# flame speeds should not be exactly equal

assert sl_mole != sl_mass

# but they should be close

assert sl_mole == approx(sl_mass, rel=0.1)

speth

Thanks, @g3bk47, I think this ended up being a pretty clean API. I just had a few small changes to recommend.

If you can rebase onto the current main branch, it will fix the CI errors.

g3bk47 · 2024-03-09T23:59:49Z

Thanks, @speth! I rebased and incorporated all requested changes.

codecov · 2024-03-10T00:17:52Z

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 72.78%. Comparing base (64ed2b7) to head (c2a9322).

Files	Patch %	Lines
interfaces/cython/cantera/_onedim.pyx	66.66%	3 Missing ⚠️
src/oneD/StFlow.cpp	88.46%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1668      +/-   ##
==========================================
+ Coverage   72.77%   72.78%   +0.01%     
==========================================
  Files         375      375              
  Lines       56632    56667      +35     
  Branches    20590    20603      +13     
==========================================
+ Hits        41212    41244      +32     
- Misses      12398    12399       +1     
- Partials     3022     3024       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

speth

Thanks! Looks good to me.

g3bk47 force-pushed the flame_mix_diff_mass branch 2 times, most recently from 87248d7 to 19a4da1 Compare February 23, 2024 17:45

speth requested changes Mar 9, 2024

View reviewed changes

g3bk47 force-pushed the flame_mix_diff_mass branch from 19a4da1 to ab006a8 Compare March 9, 2024 23:56

[1D] Add option for mass fraction gradient fluxes

c2a9322

g3bk47 force-pushed the flame_mix_diff_mass branch from ab006a8 to c2a9322 Compare March 10, 2024 00:20

speth approved these changes Mar 10, 2024

View reviewed changes

speth merged commit 66835e2 into Cantera:main Mar 10, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option for mass gradient-based fluxes #1668

Add option for mass gradient-based fluxes #1668

g3bk47 commented Feb 16, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth Mar 9, 2024

speth left a comment

g3bk47 commented Mar 9, 2024

codecov bot commented Mar 10, 2024 •

edited

Loading

speth left a comment

	using the mixture-avaraged transport model.
	using the mixture-averaged transport model.

		writelog("Warning: setting fluxGradientBasis only affects"
		" the mixture-averaged diffusion model.");

		@@ -780,6 +804,8 @@ AnyMap StFlow::getMeta() const

		state["Soret-enabled"] = m_do_soret;

		state["Flux-Gradient-Basis"] = static_cast<long int>(m_fluxGradientBasis);

Add option for mass gradient-based fluxes #1668

Add option for mass gradient-based fluxes #1668

Conversation

g3bk47 commented Feb 16, 2024

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

speth left a comment

Choose a reason for hiding this comment

g3bk47 commented Mar 9, 2024

codecov bot commented Mar 10, 2024 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2024 •

edited

Loading