Skip to content
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

Enhance Python Func1 Interface #1521

Merged
merged 9 commits into from
Jul 1, 2023
Merged

Enhance Python Func1 Interface #1521

merged 9 commits into from
Jul 1, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jun 30, 2023

Changes proposed in this pull request

  • Provide access to all C++ Func1 objects from Python with dynamically created classes.
  • Remove unnecessary argument from advanced functor constructors (oversight in Refactor Func1 #1513)
  • Replace Python callbacks returning constants with C++ functors
  • Rename TabulatedFunction to Tabulated1 to ensure names are consistent

While the Func1 API in Python should mainly serve as an 'advanced feature', it can serve as a template for other API's (example: new MATLAB interface Cantera/enhancements#177).

If applicable, provide an example illustrating new features this pull request is introducing

In [1]: import cantera as ct

In [2]: f1 = ct.Func1.cxx_functor("sin")
   ...: f2 = ct.Func1.cxx_functor("Gaussian", [0, 1, 2])
   ...: func = ct.Func1.cxx_functor("sum", f1, f2)

In [3]: func
Out[3]: <cantera.func1.Sum1 at 0x10457cd00>

In [4]: print(func.write())
\sin(t) + \mathrm{Gaussian}(t)

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@ischoegl ischoegl changed the title Enhance Python func1 Interface Enhance Python Func1 Interface Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #1521 (95ca64a) into main (03f5b82) will increase coverage by 0.06%.
The diff coverage is 85.47%.

@@            Coverage Diff             @@
##             main    #1521      +/-   ##
==========================================
+ Coverage   70.52%   70.59%   +0.06%     
==========================================
  Files         376      376              
  Lines       58918    58956      +38     
  Branches    21198    21197       -1     
==========================================
+ Hits        41551    41619      +68     
+ Misses      14308    14264      -44     
- Partials     3059     3073      +14     
Impacted Files Coverage Δ
include/cantera/numerics/Func1.h 49.55% <ø> (+1.47%) ⬆️
src/clib/ctfunc.cpp 32.71% <33.33%> (ø)
src/numerics/Func1.cpp 33.45% <84.37%> (+5.49%) ⬆️
interfaces/cython/cantera/func1.pyx 90.47% <85.96%> (-9.53%) ⬇️
src/numerics/Func1Factory.cpp 96.70% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@speth
Copy link
Member

speth commented Jun 30, 2023

I'm not really clear on the use case for constructing Func1 objects in Python, except for Func1Py which allows calling arbitrary Python functions. The latter makes it easy to define both simple functions as lambdas as well as arbitrarily complex functions including capturing outside variables, which the piecewise Func1 construction can never replicate.

@ischoegl ischoegl requested a review from a team June 30, 2023 14:15
@ischoegl
Copy link
Member Author

@speth ... this is ready for a review. For the Python Func1 API, I kept the access outside of the main constructor. On of my initial reasons is that it is possible to recreate the C++ type names dynamically (see SO example), but I'm not sure that it's worth the effort. Another reason is that I want to keep this an "advanced feature" with the current Python callback behavior remaining the go-to solution.

@ischoegl
Copy link
Member Author

ischoegl commented Jun 30, 2023

I'm not really clear on the use case for constructing Func1 objects in Python, except for Func1Py which allows calling arbitrary Python functions. The latter makes it easy to define both simple functions as lambdas as well as arbitrarily complex functions including capturing outside variables, which the piecewise Func1 construction can never replicate.

I am aware that the Func1Py approach is a lot more flexible. This PR (where most lines are actually a clean-up for #1513) mainly illustrates that it is extremely simple to add access. Python can serve as a test-bed for other API's that lack the generic callbacks (e.g. MATLAB, or prototyping for C++ implementations). In theory, the C++ functors should also be faster than Python callbacks, although I'm not sure how much this matters. Regardless, I replaced the constant lambda functions with the C++ equivalent.

@ischoegl
Copy link
Member Author

ischoegl commented Jun 30, 2023

@speth ... I renamed the factory class method to cxx_functor; the factory recreates the underlying C++ classes dynamically, where type names are preserved.

Ad use case: the Python Func1 API will allow for assigning dedicated C++ functors from the Python interface. Beyond this PR, there is no additional Cython code necessary (all classes are created dynamically).

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. I'm glad to see that this incorporates the simplification I had been trying to get at in #1513 to eliminate the unnecessary integer argument to the Func1 constructors. Besides that, just a couple of very minor comments.

doc/sphinx/cython/zerodim.rst Show resolved Hide resolved
include/cantera/numerics/Func1.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Jul 1, 2023

Thanks, @speth for the prompt review. I can retain the old name for TabulatedFunction, but explained why I opted to update. Let me know ...

PS: there is one newly introduced ct.TabulatedFunction in a test in #1515, which I'll address based on what is merged first.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl, I think this is good to go.

@speth speth merged commit b4dad59 into Cantera:main Jul 1, 2023
@ischoegl ischoegl deleted the python-func1 branch July 1, 2023 18:36
@ischoegl ischoegl mentioned this pull request Jul 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants