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

Refactor Func1 #1513

Merged
merged 23 commits into from
Jun 29, 2023
Merged

Refactor Func1 #1513

merged 23 commits into from
Jun 29, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jun 23, 2023

Changes proposed in this pull request

Functors are presumably the easiest pathway to create custom functions using API's that are based on clib. This PR seeks to update the framework to facilitate future usage.

  • Replace Func1& references by shared_ptr<Func1> (avoids raw pointers and requirement to copy objects for various operations)
  • Implement factory constructors
  • Improve unit test coverage
  • Eliminate Cabinet
  • Create new clib functions that do not rely on hardcoded magic numbers; magic numbers are, however, not deprecated as they are still used by the stable MATLAB toolbox.
  • Eliminate one loophole potentially causing segfaults in SharedCabinet

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

Closes Cantera/enhancements#164

Addresses #752, #567

Directly applicable to Cantera/enhancements#177

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

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #1513 (4983a88) into main (36faa94) will increase coverage by 0.07%.
The diff coverage is 54.84%.

@@            Coverage Diff             @@
##             main    #1513      +/-   ##
==========================================
+ Coverage   70.42%   70.49%   +0.07%     
==========================================
  Files         375      376       +1     
  Lines       58440    58891     +451     
  Branches    20919    21193     +274     
==========================================
+ Hits        41155    41516     +361     
- Misses      14262    14316      +54     
- Partials     3023     3059      +36     
Impacted Files Coverage Δ
src/clib/ctreactor.cpp 6.25% <ø> (ø)
src/numerics/Func1.cpp 27.96% <43.31%> (+17.08%) ⬆️
src/clib/ctfunc.cpp 32.07% <50.00%> (+4.07%) ⬆️
src/clib/Cabinet.h 55.07% <66.66%> (+6.95%) ⬆️
include/cantera/numerics/Func1.h 48.08% <67.56%> (+30.34%) ⬆️
src/numerics/Func1Factory.cpp 96.70% <96.70%> (ø)

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

@ischoegl ischoegl force-pushed the refactor-func1 branch 2 times, most recently from 63a1047 to 3408368 Compare June 24, 2023 00:58
@ischoegl ischoegl marked this pull request as ready for review June 24, 2023 01:59
@ischoegl ischoegl requested a review from a team June 24, 2023 02:00
@ischoegl ischoegl force-pushed the refactor-func1 branch 6 times, most recently from 035b5f6 to cf3c529 Compare June 24, 2023 13:48
@ischoegl
Copy link
Member Author

ischoegl commented Jun 24, 2023

@speth ... at this point, this is ready for a review. While the line count is a bit larger than expected (a lot of this is due to deprecations, new unit tests and refactored code that otherwise doesn't introduce anything new), this PR should remove a lot of lint from the Func1 interface, which will be a lot simpler after the deprecations flushed out. As an example, I created a new Log1 functor, which only required a handful of lines.

The central enhancement are new factory functions (while eliminating the need for magic numbers), where I stuck to MATLAB nomenclature for the time being. Every single Func1 class is accessible from clib at this point, which I hope will be propagated into the new experimental MATLAB interface. Rather than using a single clib (or C++) function signature, I grouped as appropriate, which should make things simpler to use going forward.

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 for taking the time to clean up this interface. It was past due for an overhaul. Besides moving clib away from magic numbers, I appreciate especially the elimination of the manual memory management with backwards semantics (like returning references to objects that the caller was then responsible for deleting) and the expansion of the test suite.

include/cantera/clib/ctfunc.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
src/numerics/Func1.cpp Outdated Show resolved Hide resolved
src/clib/ctfunc.cpp Outdated Show resolved Hide resolved
include/cantera/numerics/Func1.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1.h Outdated Show resolved Hide resolved
test/clib/test_ctfunc.cpp Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the refactor-func1 branch 5 times, most recently from 6abc356 to b05df1f Compare June 27, 2023 15:56
@ischoegl
Copy link
Member Author

ischoegl commented Jun 27, 2023

@speth ... thanks for the review! I definitely appreciate your suggestions, and implemented almost everything. There is only one minor item left: I can't really think of a good name for the miscellaneous category (other than 'other', which is not an improvement). Everything else should be taken care of.

@ischoegl
Copy link
Member Author

Rebased due to merge conflict with #1511

@ischoegl
Copy link
Member Author

ischoegl commented Jun 27, 2023

Resolved the final comment.

Edit: also renamed func_new_basic to func_new_essential in clib.

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 for the updates, @ischoegl.

I like the new organization for the related Doxygen documentation. However, this still doesn't inform the user what name to pass to newFunc1 or func_new_xyz to get a function of a particular type, so I think we need to find a home for these lists in the documentation, even if they risk being out of date if new Func1 classes are added. I'd say that this is somewhat analogous to the list of thermo models that we maintain here.

include/cantera/clib/ctfunc.h Outdated Show resolved Hide resolved
include/cantera/clib/ctfunc.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1.h Outdated Show resolved Hide resolved
include/cantera/numerics/Func1Factory.h Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Func/Func.m Show resolved Hide resolved
@ischoegl
Copy link
Member Author

@speth ... thanks! I believe I took care of everything.

I like the new organization for the related Doxygen documentation. However, this still doesn't inform the user what name to pass to newFunc1 or func_new_xyz to get a function of a particular type, so I think we need to find a home for these lists in the documentation, even if they risk being out of date if new Func1 classes are added. I'd say that this is somewhat analogous to the list of thermo models that we maintain here.

I agree in general. At the same time, the generated doxygen documentation actually does a really good job summarizing everything, although it's probably not easy to find. As the code is largely absent from Python, this would have to be done manually on the website ...

image

@speth
Copy link
Member

speth commented Jun 28, 2023

Right, I think this documentation page is clear that newFunc1("???", c) is what should be called to create a new Const1 function, but how is the user supposed to know that ??? should be constant instead of const or Const1 or something else? This is almost obvious for a lot of the function types, but not all of them.

If you don't want a list in a single place, then an example in each derived class's docstring would be an alternative option.

@ischoegl
Copy link
Member Author

ischoegl commented Jun 29, 2023

@speth ... I added lists of available types to each of the four doxygen groupings, and mentioned the type of each class in (improved) class docstrings. I hope this addresses the remaining comment 😄

@ischoegl ischoegl requested a review from speth June 29, 2023 02:38
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. This looks good to me. Sorry for all the nitpicking on the documentation.

@speth speth merged commit 2b5e7e5 into Cantera:main Jun 29, 2023
@ischoegl ischoegl deleted the refactor-func1 branch June 29, 2023 12:43
@ischoegl ischoegl mentioned this pull request Jun 30, 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.

Replace Cabinet by SharedCabinet in clib
2 participants