Add `SolutionArray :set_mixture_fraction` method. by decaluwe · Pull Request #1242 · Cantera/cantera

decaluwe · 2022-04-14T23:19:42Z

Changes proposed in this pull request

Adds the capability to set the mixture fraction of a solutionArray.

This largely follows the method already implemented for solutionArray:set_equivalence_ratio.

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

Addresses Cantera/enchancements#147

As noted in the enhancement request, an example implementation is:

import cantera as ct

yfuel = {'C': 43.41, 'H': 5.82, 'O': 44.32, 'N': 0.25}
yair = {'O2': 23, 'N2': 76, 'Ar': 1}
mixfrac = np.linspace(0.1, 0.9, 20)

gas = ct.Solution('gri30.yaml')
states = ct.SolutionArray(gas, shape=len(mixfrac))
states.TP = 900, ct.one_atm
states.set_mixture_fraction(mixfrac, yfuel, yair, basis='mass')
states.equilibrate('TP')

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

addresses Cantera/enhancements#147

decaluwe · 2022-04-14T23:25:30Z

@ischoegl I want to add a test to cover this (and will cover set_equivalence_ratio as well), then it should be ready for review.

bryanwweber · 2022-04-15T00:01:51Z

interfaces/cython/cantera/composite.py

+        mixture_fraction, _ = np.broadcast_arrays(mixture_fraction, 
+            self._output_dummy)
+
+        # loop over values


A nit, but this comment and the one above don't add anything to the explanation. I see they're copied from the other method, but I don't see a reason to copy them over.

Certainly for line 1016.

For line 1013, it is not entirely clear to me what line 1012 does. My best guess: if mixture_fraction is a scalar, we convert it to an array of the correct size, where are elements are set to the value mixture_fraction. Is that correct?

To put a finer point on it: @bryan, would you be supportive of me adding a comment that actually explains what line 1012 does (while deleting the comment on line 1016)?

Yes, I think that would be perfect, thank you 😊

Okay, but so: is my understanding above of line 1012 correct? 😂

Deletes an unnecessary comment and make an additional comment more descriptive, both in set_equivalence_ratio and set_mixture_fraction

bryanwweber · 2022-04-17T03:44:37Z

interfaces/cython/cantera/composite.py

        """

-        # broadcast argument shape
+        # If ``phi`` is a scalar, broadcast it to the correct argument shape:


Thanks for taking a shot at improving this! I think phi can be any shape that's compatible with self._output_dummy. I think the key is that phi needs to have a lower dimensionality than the other array, but I'm not totally sure. I'd have to read the docs for numpy to see, I can never remember how broadcasting works.

The one-line version in my head is

Broadcast phi to match the output shape

But I don't think that adds much either. It's basically restating the name of the function without any more explanation about why this needs to be done.

I think I'll try to grok the Numpy docs and see if there's an alternative that comes to mind.

Fwiw, and if memory serves, I used this function upon suggestion when implementing the set equivalence ratio method. I believe @bryanwweber is describing the behavior correctly: it just makes sure that dimensions are matched. We mostly use 1d where things are simple. For 2d and above, broadcasting adds dimensions based on established rules.

But I don't think that adds much either. It's basically restating the name of the function without any more explanation about why this needs to be done.

You could write:

If phi is lower-dimensional than the SolutionArray's shape (for example, a scalar), broadcast it to have the right number of dimensions.

decaluwe · 2022-04-17T10:38:38Z

Thanks—I read the docs earlier, even looked at some examples, and _still_ can’t figure out what/why it does 😂

…

Sent from my iPhone

On Apr 17, 2022, at 1:44 PM, Bryan Weber ***@***.***> wrote: @bryanwweber commented on this pull request. In interfaces/cython/cantera/composite.py: > @@ -992,15 +992,33 @@ def set_equivalence_ratio(self, phi, *args, **kwargs): to be matched to the `SolutionArray`. """ - # broadcast argument shape + # If ``phi`` is a scalar, broadcast it to the correct argument shape: Thanks for taking a shot at improving this! I think phi can be any shape that's compatible with self._output_dummy. I think the key is that phi needs to have a lower dimensionality than the other array, but I'm not totally sure. I'd have to read the docs for numpy to see, I can never remember how broadcasting works. The one-line version in my head is Broadcast phi to match the output shape But I don't think that adds much either. It's basically restating the name of the function without any more explanation about why this needs to be done. I think I'll try to grok the Numpy docs and see if there's an alternative that comes to mind. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

codecov · 2022-04-21T03:29:20Z

Codecov Report

Merging #1242 (21ce0e2) into main (e7b931b) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1242      +/-   ##
==========================================
- Coverage   65.47%   65.47%   -0.01%     
==========================================
  Files         327      327              
  Lines       46416    46424       +8     
  Branches    19718    19726       +8     
==========================================
+ Hits        30391    30396       +5     
- Misses      13496    13497       +1     
- Partials     2529     2531       +2

Impacted Files	Coverage Δ
src/kinetics/KineticsFactory.cpp	`90.56% <0.00%> (-1.36%)`	⬇️
src/thermo/ThermoFactory.cpp	`71.19% <0.00%> (-0.20%)`	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ischoegl · 2022-04-21T04:17:57Z

I want to add a test to cover this (and will cover set_equivalence_ratio as well), then it should be ready for review.

@decaluwe … this looks really good! I think once the unit test is added, this is ready to go.

decaluwe · 2022-04-22T04:34:32Z

Thanks, @ischoegl - just added! Once it passes build 🤞, please have a look!

The new set_mixture_fraction tests use the assertArrayNear and assertRaisesRegex methods for better comparisons. Split the test into several methods so failures can be more easily diagnosed.

Improve the set_equivalence_ratio tests by using the assertArrayNear and assertRaisesRegex methods to perform more specific comparisons. Split the tests into several functions to ease debugging of failures.

bryanwweber · 2022-04-22T14:27:11Z

Thanks @decaluwe I made some changes to the test_set_mixture_fraction() methods. Please feel free to drop those changes if you don't agree (I figured this would be faster than going back-and-forth in a review).

ischoegl

@decaluwe thanks! ... also thanks to @bryanwweber for beating me to a review. I think this is ready ...

bryanwweber · 2022-04-22T18:09:28Z

All green, in it goes! Thanks @decaluwe!

decaluwe · 2022-04-23T23:11:29Z

Thanks for the help, @bryanwweber. Certainly agree, re: just making the changes directly rather than a back and forth. I'm not particularly territorial about these things :)

Add SolutionArray :set_mixture_fraction method.

0049e3d

addresses Cantera/enhancements#147

decaluwe force-pushed the array_mix_frac branch from be54e47 to 0049e3d Compare April 14, 2022 23:24

decaluwe requested a review from ischoegl April 14, 2022 23:24

bryanwweber reviewed Apr 15, 2022

View reviewed changes

Improve comments in composite.py

331544e

Deletes an unnecessary comment and make an additional comment more descriptive, both in set_equivalence_ratio and set_mixture_fraction

bryanwweber reviewed Apr 17, 2022

View reviewed changes

Update broadcasting comment.

973583f

SolutionArray.set_mixture_fraction.py unit test

c84f88e

bryanwweber added 2 commits April 22, 2022 09:36

[Cython/Test] Use array tests and regex match

a29be33

The new set_mixture_fraction tests use the assertArrayNear and assertRaisesRegex methods for better comparisons. Split the test into several methods so failures can be more easily diagnosed.

[Cython/Test] Use array/regex testing methods

f5f2d17

Improve the set_equivalence_ratio tests by using the assertArrayNear and assertRaisesRegex methods to perform more specific comparisons. Split the tests into several functions to ease debugging of failures.

[Test/Cython] Fix assert raises regex

21ce0e2

ischoegl approved these changes Apr 22, 2022

View reviewed changes

bryanwweber merged commit 08602cd into Cantera:main Apr 22, 2022

decaluwe mentioned this pull request Apr 23, 2022

Set mixture fraction for SolutionArray Cantera/enhancements#147

Closed

Uh oh!

Conversation

decaluwe commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decaluwe commented Apr 14, 2022

Uh oh!

bryanwweber Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

decaluwe Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

decaluwe Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

bryanwweber Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

decaluwe Apr 16, 2022

Choose a reason for hiding this comment

Uh oh!

bryanwweber Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

ischoegl Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

speth Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

decaluwe commented Apr 17, 2022 via email

Uh oh!

codecov bot commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ischoegl commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decaluwe commented Apr 22, 2022

Uh oh!

bryanwweber commented Apr 22, 2022

Uh oh!

ischoegl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanwweber commented Apr 22, 2022

Uh oh!

decaluwe commented Apr 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

decaluwe commented Apr 14, 2022 •

edited

Loading

decaluwe Apr 15, 2022 •

edited

Loading

codecov bot commented Apr 21, 2022 •

edited

Loading

ischoegl commented Apr 21, 2022 •

edited

Loading

ischoegl left a comment •

edited

Loading