Skip to content

Use CanteraError#587

Closed
CyberDrudge wants to merge 5 commits into
Cantera:masterfrom
CyberDrudge:exception
Closed

Use CanteraError#587
CyberDrudge wants to merge 5 commits into
Cantera:masterfrom
CyberDrudge:exception

Conversation

@CyberDrudge
Copy link
Copy Markdown
Contributor

Fixes part of #569

Implemented use of ct.CanteraError in ic_engine.py

Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start, keep going with the other files!

Comment thread interfaces/cython/cantera/examples/reactors/ic_engine.py Outdated
Comment thread interfaces/cython/cantera/examples/reactors/ic_engine.py Outdated
@bryanwweber
Copy link
Copy Markdown
Member

@speth This change causes the Refinement limit reached error to be raised and the example effectively doesn't work. Prior to this change, as we've noted, the logic in the loop was not correct but at least it produced a figure at the end. What did you mean if sim.reinitialize() is used, the try-except isn't necessary? Won't the try be needed to catch when the time step isn't fine enough? Should the max time step even be adjusted here at all?

I think we need to figure out how to fix this example before this PR can be merged, otherwise we'd distribute broken examples 😄

@speth
Copy link
Copy Markdown
Member

speth commented Jan 17, 2019

The reason it doesn't work is because the logic in the loop isn't right -- the if not solved check should be outside the loop. Right now, it gets hit on the first failure, and you never make a second pass through the loop.

I think my comment about sim.reinitialize() is well beyond the scope of this PR. Because of the way the CVODES integrator works, where it can need function evaluations at times other than the specified output times, changing parameters of the ODE outside of the ODE function (where the time that the integrator chooses to evaluate is actually known) can lead to failures. One way around this is to call reinitialize after any such discontinuous change. The other is to set the parameters from a function that is called as part of the ODE evaluation, e.g. the way we allow MassFlowController to have a mass flow rate as a function of time.

@bryanwweber
Copy link
Copy Markdown
Member

@CyberDrudge Please see the first part of @speth's comment. I missed the indentation there!

@bryanwweber
Copy link
Copy Markdown
Member

@CyberDrudge I rebased the commits to combine a few of them and updated it to the current master branch. This PR was merged by d9b95b2 Thank you for doing this work!

@bryanwweber bryanwweber closed this Feb 1, 2019
@bryanwweber
Copy link
Copy Markdown
Member

@CyberDrudge Since you have this experience now, I'd encourage you to go through the rest of the places noted in #569 to fix those as well 😄

@CyberDrudge
Copy link
Copy Markdown
Contributor Author

Sure, I'll work on them as soon as i'll get some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants