-
-
Notifications
You must be signed in to change notification settings - Fork 351
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
Extensible reaction rates #1382
Conversation
@speth ... the interface looks quite promising! Regarding further extensions: in |
My plan is to provide an interface much like we use in C++: Add a Python At least so far, the biggest complication I see is the need to wrap existing C++ |
3f0b4e8
to
fa47e0b
Compare
Codecov Report
@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
- Coverage 71.04% 70.85% -0.19%
==========================================
Files 359 363 +4
Lines 51707 51837 +130
Branches 17327 17354 +27
==========================================
- Hits 36735 36729 -6
- Misses 12634 12716 +82
- Partials 2338 2392 +54
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
273e194
to
e05262d
Compare
2a7fc63
to
db89907
Compare
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.
@speth ... thank you for this substantial expansion of Cantera capabilities. I see the value not necessarily in the extensible reaction rates themselves, but more in the creation of a machinery to pull in external code extensions natively, which will presumably be expanded to other areas (and languages). The code itself looks good to me; my (nit-picky) in-line comments are mostly about formatting.
As is evident from the C++ test, the availability of the Python extension is not dependent on the installation of Cantera's Python API.
One thing that I believe would be good to have is a routine check that the SCons python_package=minimal
works (on that note, we still have the python_package=none
option). A dedicated GitHub runner for minimal
installation and C++ tests on a ubuntu/Windows/macOS
matrix would give some peace of mind (e.g. considering partial Cantera installations like conda's libcantera-devel
without the cantera
Python API).
This is not quite correct. To implement a Python extension, the user's class must derive from |
True (sadly). |
db89907
to
8dc2b9b
Compare
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 updating CONTRIBUTING.md
... within the context of Sphinx/doxygen directives, there are two other comments based on prior discussion.
Without this, the default Python path is not populated correctly, and is based on the path to the user's executable instead.
If a user application is linked statically to the Cantera library, ExtensibleRate objects need to be registered in this copy of the Cantera library rather than the one that is embedded in the Python module. This is achieved by accessing the ReactionRateFactory from the main application rather than from the Python module.
This avoids weird linker issues where the GitHub Actions Python required linkage to libintl but the only available version of libintl, installed in Homebrew, was targeting a different macOS version.
7e5da26
to
d7efc72
Compare
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, @speth - LGTM!
Looks like the changes broke
|
I don't understand why this should be a problem, given that the
I don't think this should inherently be a problem, but apparently something isn't right here. |
🤷♂️ … I don’t know why this is failing either. Some of the other build scripts that are run upon a merge (MSI, PyPI, macOS) are failing also, but I honestly have never looked at those before, and there may be other things at play. |
Oh, I see what the problem is. In the The other thing that has changed that isn't accounted for yet in the conda recipe is that |
🎉 … makes sense
When I made the (incorrect) comment about the Python API not being required for extensions to work, I was coming from a similar direction. So This probably should go into an issue report on |
If applicable, provide an example illustrating new features this pull request is introducing
I figured this should start with an example, before the details of how it works:
Create an input file:
With the corresponding module
user_ext.py
to define the rate type:This input file can now be loaded via any Cantera user interface and will call the user-defined functions for that reaction.
Changes proposed in this pull request
ReactionRateDelegator
class for delegating methods ofReactionRate
to other languages, withExtensibleRate
as the Python base class for this (mirroring the convention established withReactorDelegator
andExtensibleReactor
.ExtensionManager
base class along withExtensionManagerFactory
andPythonExtensionManager
classes that are responsible for loading extensions in other languages (e.g. Python modules) and registering theExtensibleXYZ
objects defined in these modules with the corresponding factories.@extension
decorator to the Python module that is used to register models with the corresponding factories using thePythonExtensionManager
.Delegator
class to be able to manage the lifetime of external language objects by holding "handles" to them.ReactionRate
functionality, namelyset_parameters
andeval
.setup-python
action to fix a weird linking problem related tolibintl
.If applicable, fill in the issue number this pull request is fixing
This is a major step in implementing Cantera/enhancements#79.
Work for subsequent PRs
To keep this PR a manageable size, I've deliberately limited the interface for now. In subsequent PRs, I anticipate the following further changes:
eval
interface to pass in a customizableReactionData
structure rather than just passing the temperatureReactionRate::getParameters
to enable serialization andddTScaledFromStruct
to allow derivative calculationsChecklist
scons build
&scons test
) and unit tests address code coverage