-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SolutionArray :set_mixture_fraction
method.
#1242
Conversation
be54e47
to
0049e3d
Compare
@ischoegl I want to add a test to cover this (and will cover |
mixture_fraction, _ = np.broadcast_arrays(mixture_fraction, | ||
self._output_dummy) | ||
|
||
# loop over values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that would be perfect, thank you 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 Report
@@ 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
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@decaluwe … this looks really good! I think once the unit test is added, this is ready to go. |
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.
Thanks @decaluwe I made some changes to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@decaluwe thanks! ... also thanks to @bryanwweber for beating me to a review. I think this is ready ...
All green, in it goes! Thanks @decaluwe! |
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 :) |
Changes proposed in this pull request
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:
Checklist
scons build
&scons test
) and unit tests address code coverage