-
-
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
New C++ custom ODEs example #922
Conversation
@paulblum ... great to see a new example! The current CI failures should be relatively easy to fix by specifying thresholds (just had to fix a similar thing for #927 - I admittedly didn't investigate and just copied values). Beyond, I think it may make sense to switch the mechanism from |
@ischoegl Any reason to prefer |
@bryanwweber ... it's admittedly a minor (philosophical?) point, but things do add up. I would generally advocate for using the smallest possible mechanism that will do the job especially when there is absolutely no benefit of using a larger mechanism: as PS: As another benefit, end-users (who are probably interested in some C species) will have to actually think about what mechanism to use instead of always defaulting to |
Just updated the regression test tolerances from the defaults to |
@ischoegl thanks for the feedback! I'll look into switching this mechanism next. |
2224aed
to
46b39b8
Compare
@paulblum @ischoegl I'd be in favor of deferring the switch to |
🤷♂️ ... makes no difference as long as it’ll get done. I agree that this is an easy fix ... 5ish lines spread over a couple of files? Not sure it’s worth its own PR ... PS: #932 |
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 a lot for providing this example, @paulblum! I think it looks very good, and provides a C++ example that's non-trivial but still fairly clean.
I think there are just a handful of issues to tweak, along with rebasing this and fixing the conflicts.
13745ae
to
4557613
Compare
Thanks for the review @speth, these were all really good suggestions! Just pushed a commit with the updates, please let me know if there's anything else you think can be improved 😄 |
Failed CI test due to regression test tolerances, I had fixed that in an earlier version but looks like it got removed by a conflict... latest commit should be all set! |
Thanks, @paulblum, this looks great. I think the only thing I'd like to do before merging is to squash the last couple of commits to avoid the large diff associated with the changes to the |
@speth that makes sense to me, although I'm not familiar with how to squash commits... probably better for you to do it if it's not too much trouble. Thanks again for the help! |
Switch from arrays to vectors, and incorporate review suggestions
207b149
to
e3210b4
Compare
Proposing the addition of a new example for Cantera's C++ sample code,
custom.cpp
. This example solves a closed-system constant-pressure ignition problem using custom-implemented ODE governing equations, which are solved by CVODES. The actual problem is equivalent to the one solved in the existingcustom.py
example, and produces the same results.This example was created as part of my GSoC project, Developing a 0-D Steady-State Combustion Solver for Cantera. The project is ongoing, and progress updates can be found in the enhancements repository.