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

Smart clib cabinets / add Solution to clib API #1448

Merged
merged 31 commits into from Mar 7, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Mar 1, 2023

Changes proposed in this pull request

This PR required keeping track of shared pointers to core Cantera objects (ThermoPhase, Kinetics and Transport) after their construction. Several factory constructors and internal methods that previously used raw pointers or references are replaced by methods that use shared pointers, with the original methods being deprecated.

Modifications allowed for consistent tracking of objects behind the clib API, where a new SharedCabinet based on smart pointers is introduced. The new object is adopted for ThermoCabinet, KineticsCabinet and TransportCabinet.

Other details:

  • Fix bugs in test_problems/clib_test/clib_test.c
  • Implement SolutionCabinet based on SharedCabinet
  • Implement clib tests based on googletests (run via scons test-clib-gtest), which now includes tests for ctonedim

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

Fixes #1447
Resolves Cantera/enhancements#158

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

Solution types can now be loaded directly, i.e.

int soln_newSolution(const char* infile, const char* name, const char* transport);
int soln_newInterface(const char* infile, const char* name, int na, const int* adjacent);

where parts are obtained as:

int soln_thermo(int n);
int soln_kinetics(int n);
int soln_transport(int n);

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 added the clib label Mar 1, 2023
@ischoegl ischoegl force-pushed the shared-clib-cabinets branch 2 times, most recently from 26376e1 to f771e0f Compare March 1, 2023 04:47
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #1448 (90b19b4) into main (c54e429) will increase coverage by 0.08%.
The diff coverage is 66.91%.

@@            Coverage Diff             @@
##             main    #1448      +/-   ##
==========================================
+ Coverage   70.94%   71.02%   +0.08%     
==========================================
  Files         369      369              
  Lines       55259    55558     +299     
  Branches    18207    18378     +171     
==========================================
+ Hits        39201    39462     +261     
- Misses      13593    13607      +14     
- Partials     2465     2489      +24     
Impacted Files Coverage Δ
include/cantera/base/Interface.h 100.00% <ø> (ø)
include/cantera/base/Solution.h 93.33% <ø> (ø)
include/cantera/kinetics/GasKinetics.h 100.00% <ø> (ø)
include/cantera/kinetics/InterfaceKinetics.h 85.71% <ø> (ø)
include/cantera/kinetics/Kinetics.h 31.85% <ø> (ø)
include/cantera/oneD/Boundary1D.h 61.76% <ø> (ø)
include/cantera/oneD/StFlow.h 100.00% <ø> (ø)
include/cantera/thermo/ThermoPhase.h 44.58% <ø> (ø)
src/clib/clib_utils.h 21.42% <ø> (ø)
src/clib/ctfunc.cpp 0.00% <ø> (ø)
... and 53 more

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

@ischoegl ischoegl force-pushed the shared-clib-cabinets branch 11 times, most recently from 385b244 to 278203c Compare March 2, 2023 13:45
Mole fractions have to be set before T and P; otherwise, the pressure
changes. Before the change, the pressure was significantly lower than
5 atm. Also fix incorrect use of 'kin_newFromFile', which causes
unintended linkages to default objects that do not raise exceptions
but may lead to erroneous results.
@ischoegl ischoegl force-pushed the shared-clib-cabinets branch 2 times, most recently from b953085 to 331b48d Compare March 3, 2023 03:00
Unit tests cover 'clib' from within C++ googletest code
- port test-clib to google tests
- rename pre-existing version to test-clib-demo
- add tests indirectly probing SharedCabinet
- add tests for Solution access routines
- add tests for ctonedim
As the storage m_table can contain duplicate entries referencing the
same object, all entries need to be stored in the reverse lookup table.
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. Besides the minor suggestions below, I think the one thing that still needs a little bit of work is the error handling for soln_kinetics, soln_transport, and soln_adjacent. I commented on that here: #1448 (comment), but GitHub is probably trying to hide that from you.

src/kinetics/InterfaceKinetics.cpp Outdated Show resolved Hide resolved
src/kinetics/KineticsFactory.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Mar 6, 2023

@speth ... thanks for the re-review! You probably noticed that I updated the reverse lookup table to prevent memory leaks (see c5bf0db). Otherwise, I believe the clib exception handling should now be addressed. I am stuck on the m_solution->registerChangedCallback crash (see #1448 (comment)), as I'm not sure why this is causing issues.

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. The last commit I pushed took care of the callback-related errors, so I'll leave the final merge to you.

@ischoegl ischoegl merged commit 399e1cb into Cantera:main Mar 7, 2023
@ischoegl
Copy link
Member Author

ischoegl commented Mar 7, 2023

Thank you, @speth!

@ischoegl ischoegl deleted the shared-clib-cabinets branch March 7, 2023 00:02
speth added a commit to speth/cantera that referenced this pull request Aug 22, 2023
The shared_ptr goes out of scope at the end of the function,
deleting the held ThermoPhase right as it's returned.

Erroneous behaviour introduced in f3e840d (Cantera#1448).
speth added a commit to speth/cantera that referenced this pull request Aug 22, 2023
The shared_ptr goes out of scope at the end of the function,
deleting the held ThermoPhase right as it's returned.

Erroneous behavior introduced in f3e840d (Cantera#1448).
ischoegl pushed a commit that referenced this pull request Aug 22, 2023
The shared_ptr goes out of scope at the end of the function,
deleting the held ThermoPhase right as it's returned.

Erroneous behavior introduced in f3e840d (#1448).
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.

clib sim1D_save/restore broken Expose C++ Solution to clib
2 participants