Make test_mass_flow_controller more robust#1043
Conversation
ischoegl
left a comment
ischoegl
left a comment
There was a problem hiding this comment.
I believe that changing the test is ultimately better than fighting some compiler-specific symptoms.
|
|
||
| mfc = ct.MassFlowController(reservoir, self.r1) | ||
| mfc.mass_flow_rate = lambda t: 0.1 if 0.2 <= t < 1.2 else 0.0 | ||
| # Triangular pulse with area = 0.1 |
There was a problem hiding this comment.
While I usually would type it like this also, I believe Cantera uses white spaces left and right of * (on line 447 below what is marked here).
There was a problem hiding this comment.
Fair enough. It doesn't fit on one line anymore, but 🤷 .
There was a problem hiding this comment.
I think you could get it to fit on a line by reversing the check of the time and returning zero first. Then you could dedent the calculation line
There was a problem hiding this comment.
Oh, I see now that I'm looking on a computer. Ignore this comment 😂
speth
force-pushed
the
debug-mfc-reactor-test-v3
branch
from
d13bf8b to
6a37440
Compare
|
I’m still tagging @bryanwweber so he gets a chance to respond |
bryanwweber
left a comment
bryanwweber
left a comment
There was a problem hiding this comment.
Looks good to me. Can you add a docstring to the test including the discussion from #1033? Then we don't need to have this conversation again 😊
|
What part of that discussion? I don't think there's much that really belongs in the code, as it becomes an exercise in explaining what isn't there. Perhaps some of what I wrote in my last comment belongs in the commit message. |
|
I think adding a note that the function is intentionally discontinuous in the slope to stretch the integrator is worthwhile. Basically, a note about the point of the test, why does it exist in this way. Since the implemented function choice is a little arbitrary, someone in the future may come along and wonder why it's not a smoother function. I agree that a justification for the change from square to triangle belongs in the commit message, though. |
…oller Use a triangular pulse in the mass flow rate instead of a square wave so that the function is continuous (but not necessarily smooth). This still shows that CVODES is able to appropriately reduce the step size around such a feature in the input function, but seems to be give results that are more consistent across different systems / library versions. Fixes Cantera#1033.
speth
force-pushed
the
debug-mfc-reactor-test-v3
branch
from
6a37440 to
2ea72c2
Compare
|
|
||
| mfc = ct.MassFlowController(reservoir, self.r1) | ||
| mfc.mass_flow_rate = lambda t: 0.1 if 0.2 <= t < 1.2 else 0.0 | ||
| # Triangular pulse with area = 0.1 |
There was a problem hiding this comment.
Oh, I see now that I'm looking on a computer. Ignore this comment 😂
|
Well, I guess all of these minor edits have gotten us several additional CI runs, so it's pretty clear that this is fixed, at least for the current version of OpenBLAS. |
3 participants
This is an alternative to #1035 and #1039.
As I noted in #1033, this test is (deliberately) somewhat rough on the ODE solver. We are providing a mass flow rate function that's a square wave, so it has detect that it's not resolving something at the point where the square wave switches and (repeatedly) reduce the timestep.
This change modifies the test so that the imposed mass flow rate is at least continuous, if not smooth (we still want the ODE solver to have something to chew on). I ran the GH Actions for this 4 times and haven't seen a failure yet, so I think this may be a more reliable solution than getting into a fight with OpenBLAS.
Changes proposed in this pull request
If applicable, fill in the issue number this pull request is fixing
Closes #1033
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build&scons test) and unit tests address code coverage