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

Remove magic number support for Func1 in clib #1741

Merged
merged 14 commits into from
Jul 31, 2024

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jul 27, 2024

Changes proposed in this pull request

  • Remove instantiation of Func1 via magic number from clib (no longer needed after removal of legacy MATLAB toolbox in Remove legacy MATLAB toolbox #1670)
  • Fix implementation of Poly1 (implementation did not match docstring; now, order matches NumPy and MATLAB); use transitional names Poly13 and polynomial3 to make changes tractable
  • Fix non-functional func_write in clib
  • Skip some deprecation cycles where transitional names were only relevant to legacy MATLAB toolbox
  • Implement checkFunc1 to check for existence of functors
  • Remove unnecessary raw pointers from Func1 code

Experimental MATLAB toolbox:

  • Switch to new Func1 code base
  • Use C++ implementation of string output (instead of redundant MATLAB code)
  • Eliminate trivial helper functions
  • Avoid hard-coding of functor names - use logic based on argument types instead
  • Rename Func to Func1 in order to be consistent with C++ and Python API's

Python API:

  • Improved exception handling for Func1.cxx_functor

If applicable, fill in the issue number this pull request is fixing

Closes #567

Examples

In the experimental MATLAB toolbox:

>> a = Func1('polynomial3', [1 -1 0 2.3 0 0])
 
a = 
 
   x^5 - x^4 + 2.3x^2
 
>> [a(1.5), polyval([1 -1 0 2.3 0 0], 1.5)]

ans =

    7.7062    7.7062

>> b = Func1('pow', .5) + 3 * Func1('sin')
 
b = 
 
   \sqrt{x} + 3 \sin(x)

>> [b(3), sqrt(3) + 3*sin(3)]

ans =

    2.1554    2.1554

>> c = (Func1('pow', .5) + 3) * Func1('sin')
 
c = 
 
   \left(\sqrt{x} + 3\right) \sin(x)

>> [c(3), (sqrt(3) + 3)*sin(3)]

ans =

    0.6678    0.6678

>> Func1('spam')
Error using Func1
Functor 'spam' is not implemented

In Python:

In [1]: import cantera as ct
   ...: import numpy as np

In [2]: fcn = ct.Func1.cxx_functor('polynomial3', [1, -1, 0, 2.3, 0, 0])

In [3]: fcn(1.5), np.polyval([1, -1, 0, 2.3, 0, 0], 1.5)
Out[3]: (7.706249999999999, 7.706249999999999)

In [4]: fcn.write('x')
Out[4]: 'x^5 - x^4 + 2.3x^2'

In [5]: ct.Func1.cxx_functor('spam', 1, 3)
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
Cell In[5], line 1
----> 1 ct.Func1.cxx_functor('spam', 1, 3)

File /Volumes/Data/work/GitHub/cantera/build/python/cantera/func1.pyx:128, in cantera.func1.Func1.cxx_functor()
    126 cdef vector[double] arr
    127 if CxxHasFunc1(cxx_string) == 0:
--> 128     raise NotImplementedError(f"Functor '{functor_type}' is not implemented.")
    129 if len(args) == 0 and CxxHasFunc1(cxx_string) == 1:
    130     # simple functor with no parameter

NotImplementedError: Functor 'spam' is not implemented.

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

Copy link

codecov bot commented Jul 27, 2024

Codecov Report

Attention: Patch coverage is 92.04545% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.13%. Comparing base (5893f3f) to head (4e761a7).

Files Patch % Lines
src/numerics/Func1.cpp 88.23% 2 Missing and 2 partials ⚠️
src/clib/ctfunc.cpp 50.00% 2 Missing ⚠️
interfaces/cython/cantera/func1.pyx 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
+ Coverage   73.04%   73.13%   +0.09%     
==========================================
  Files         381      381              
  Lines       54164    54134      -30     
  Branches     9240     9222      -18     
==========================================
+ Hits        39562    39590      +28     
+ Misses      11633    11587      -46     
+ Partials     2969     2957      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 2 times, most recently from febb5aa to 9233036 Compare July 28, 2024 11:22
@ischoegl ischoegl marked this pull request as ready for review July 28, 2024 11:31
@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 7 times, most recently from 0ee0ed4 to 998519b Compare July 28, 2024 13:44
@ischoegl ischoegl requested a review from a team July 28, 2024 14:05
@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 3 times, most recently from b104e37 to 368b263 Compare July 28, 2024 19:07
@ischoegl
Copy link
Member Author

Ok. I think this is finally ready for a review.

Copy link
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.

Thanks @ischoegl! I don't know enough to say whether this is good, so I just had 2 small comments.

include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
src/numerics/Func1.cpp Show resolved Hide resolved
@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 2 times, most recently from 1bf5912 to 1ee2d1b Compare July 29, 2024 16:17
@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 2 times, most recently from f0bc0e3 to 184f5a0 Compare July 29, 2024 17:57
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 some worthwhile cleanup, especially for the Matlab side of things. My one significant concern is with how we need to handle reversing the coefficient ordering for Poly1.

include/cantera/numerics/Func1.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/func1.pyx Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Utility/ctString.m Outdated Show resolved Hide resolved
src/numerics/Func1.cpp Show resolved Hide resolved
@bryanwweber bryanwweber removed their request for review July 30, 2024 01:14
@ischoegl
Copy link
Member Author

ischoegl commented Jul 30, 2024

@speth ... thanks for the review: I addressed all points.

Beyond, I realized that there were still some unnecessary raw pointers left in the Func1 interface, which I decided to remove as well (I believe those go back to a version when Cabinets weren't set up for shared pointers, which has since changed). I decided for direct removal of associated constructors for "compounding" and "modifying" functors as they are rather obscure and unlikely to be used in user code.

@ischoegl ischoegl requested a review from speth July 30, 2024 10:10
@ischoegl ischoegl force-pushed the remove-magic-Func1 branch 2 times, most recently from aa11184 to c9e6213 Compare July 31, 2024 16:15
Introduce transitional func_write3, as old API was non-functional.
Also, remove trivial helper functions for functor creation
(gaussian, polynom, fplus, ftimes and frdivide)
Remove func_new and skip deprecation as it was only used by the
legacy MATLAB toolbox.
Ensure that implementation matches docstring; now uses same order as
MATLAB or NumPy.

Use transitional name to prevent unintended behavior in user code.
Skip deprecation cycle for changed func_write as old API was broken.
Skip deprecation cycle as transitional name was only relevant for
legacy MATLAB toolbox.
Func1 is consistent with nomenclature used in C++ and Python API's.
@ischoegl ischoegl merged commit 84af00b into Cantera:main Jul 31, 2024
37 of 48 checks passed
@ischoegl
Copy link
Member Author

Thanks, @speth. The new CI failures are fixed in #1746, specifically 0d1d05d, so I am merging regardless.

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

Successfully merging this pull request may close these issues.

Missing functors in matlab toolbox
3 participants