[1D] use midpoint properties for diffusion fluxes

g3bk47 commented 9 months ago

Changes proposed in this pull request

Evaluates density and mean molecular weight at the grid midpoint location for the expression of diffusion fluxes for the mixture-averaged and unity Lewis number model.

If applicable, provide an example illustrating new features this pull request is introducing

Further discussion in https://github.com/Cantera/enhancements/issues/187.

Checklist

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

codecov[bot] commented 9 months ago

Codecov Report

Merging #1626 (df5f7af) into main (baf28af) will decrease coverage by 0.01%. Report is 2 commits behind head on main. The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1626      +/-   ##
==========================================
- Coverage   72.71%   72.70%   -0.01%     
==========================================
  Files         370      370              
  Lines       56286    56287       +1     
  Branches    20368    20369       +1     
==========================================
- Hits        40927    40925       -2     
- Misses      12363    12365       +2     
- Partials     2996     2997       +1

Files	Coverage Δ
include/cantera/oneD/StFlow.h	`100.00% <ø> (ø)`
src/oneD/StFlow.cpp	`88.30% <100.00%> (+0.03%)`	:arrow_up:
src/oneD/IonFlow.cpp	`50.27% <20.00%> (-0.28%)`	:arrow_down:

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

g3bk47 commented 9 months ago

Without touching the legacy reference files, I think relaxing the tolerances for the two legacy tests might be the easiest solution.

ischoegl commented 9 months ago

Without touching the legacy reference files, I think relaxing the tolerances for the two legacy tests might be the easiest solution.

I'm not opposed to this fix to the legacy test, although the tolerances are rather large (it’s still not an issue, as the main concern for these tests is that it remains possible to import legacy files).

Regarding the review itself, I'd like to defer to @speth, as the proposed changes introduce differences of results. The enthalpy plot in https://github.com/Cantera/enhancements/issues/187#issuecomment-1732407678 does let me believe that we should adopt the updates. I thought I recalled seeing some more larger discrepancies reported earlier (I think there were some edits since), but changes are now reasonable (<1%, per current reports in Cantera/enhancements#187).

Fwiw, it would be interesting to see how changes compare to results from other software, e.g. Chemkin / FlameMaster / zero-RK (I don't have any of these installed myself).

g3bk47 commented 9 months ago

I'm not opposed to this fix to the legacy test, although the tolerances are rather large (it’s still not an issue, as the main concern for these tests is that it remains possible to import legacy files).

I think the very large tolerances (1e-1) are only due to some species mole fractions being very close to zero (e.g. comparing X = 1e-12 to 1e-13). I could play around with both the relative and absolute tolerances, but I wanted to keep the introduced changes a minimal as possible.

I thought I recalled seeing some more larger discrepancies reported earlier (I think there were some edits since)

Yes, my initial commit only modified the diffusion coefficient of the 0ths species because I forgot to include the loop over all species (see the oldest version of my comment at https://github.com/Cantera/enhancements/issues/187#issuecomment-1732394253). But since this was just a mistake in the code, I edited the comment to include the correct values.

Fwiw, it would be interesting to see how changes compare to results from other software, e.g. Chemkin / FlameMaster / zero-RK (I don't have any of these installed myself).

I will ask my colleagues to see if anyone has access to Chemkin.

I think you can set the default values of tol_T and tol_X to None, which will use the default values in pytest.approx

I chose the default values to not change the original behavior of the tests (as these values were hard-coded before). I now changed them to None (at least locally for me, all tests still pass).

I think we should update the comment to say that the reduced tolerances are mainly to account for the change in the finite difference discretization between Cantera 2.6 and Cantera 3.1.

Comments are now included.

Once the enthalpy issue is addressed, I think we should also include a new test that explicitly checks the conservation properties of a unity Lewis, freely propagating flame, i.e.:

# species
for e in gas.element_names:
    assertEqual(np.max(f.elemental_mass_fraction(e)), np.min(f.elemental_mass_fraction(e)))

# continuity
assertEqual(np.max(f.density*f.velocity), np.min(f.density*f.velocity))

# energy
assertEqual(np.max(f.enthalpy_mass), np.min(f.enthalpy_mass))

Same could be done for a non-unity Lewis number flame by comparing the corresponding first(inlet) and last(outlet) values.

Cantera / cantera

[1D] use midpoint properties for diffusion fluxes #1626

Codecov Report