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

Fix inconsistent string handling in clib #1760

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 5, 2024

Changes proposed in this pull request

  • Fix instances where clib functions use inconsistent string handling, i.e. ct_getDataDirectories and thermo_report
  • Replace by ct_getDataDirectories3 and thermo_report3 that have buffer length and char buffer in last two positions.
  • Add threshold to MATLAB thermo report for more compact output (consistent with Python)

The change ensures that a single helper function ctString.m can be used as the interface to pass strings from C++ to MATLAB.

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

Experimental MATLAB now shows compact report (suppressing minor species):

>> gas = Solution('gri30.yaml')

  gri30:

       temperature   300 K
          pressure   1.0133e+05 Pa
           density   0.081894 kg/m^3
  mean mol. weight   2.016 kg/kmol
   phase of matter   gas

                          1 kg             1 kmol     
                     ---------------   ---------------
          enthalpy             26469             53361  J
   internal energy       -1.2108e+06        -2.441e+06  J
           entropy             64910        1.3086e+05  J/K
    Gibbs function       -1.9447e+07       -3.9204e+07  J
 heat capacity c_p             14311             28851  J/K
 heat capacity c_v             10187             20536  J/K

                      mass frac. Y      mole frac. X     chem. pot. / RT
                     ---------------   ---------------   ---------------
                H2                 1                 1           -15.717
     [  +52 minor]                 0                 0  

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 marked this pull request as ready for review August 5, 2024 18:18
@ischoegl ischoegl requested a review from a team August 5, 2024 18:20
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.12%. Comparing base (9d0e54a) to head (f8dc0ab).

Files Patch % Lines
src/clib/ct.cpp 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1760      +/-   ##
==========================================
- Coverage   73.12%   73.12%   -0.01%     
==========================================
  Files         381      381              
  Lines       54175    54172       -3     
  Branches     9226     9223       -3     
==========================================
- Hits        39615    39612       -3     
+ Misses      11600    11599       -1     
- Partials     2960     2961       +1     

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

@speth
Copy link
Member

speth commented Aug 6, 2024

What level of compatibility are we trying to maintain for the clib API between Cantera 3.0 and 3.1? So far, I think most of the changes have been consistent with what we do in the Python / C++ interface where user code should continue to work as long as it wasn't using any functions that were deprecated in Cantera 3.0. However, I'd also be OK with adopting a less conservative approach for clib to enable more rapid improvements.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 6, 2024

What level of compatibility are we trying to maintain for the clib API between Cantera 3.0 and 3.1? So far, I think most of the changes have been consistent with what we do in the Python / C++ interface where user code should continue to work as long as it wasn't using any functions that were deprecated in Cantera 3.0. However, I'd also be OK with adopting a less conservative approach for clib to enable more rapid improvements.

Good question. With the legacy MATLAB toolbox gone, I believe that all of the remaining API's are de-facto "experimental" (.NET is unfinished and Fortran only captures a small subset of features). We can certainly include placeholders so symbols are still in place, but at the same time breaking changes may be a feature. Specifically, I think it's dangerous to use the 3.1 version of the experimental MATLAB toolbox with a 3.0 clib (or, more realistically, a 3.0 experimental toolbox with 3.1 clib). Until we have non-experimental API's that run through clib, I believe we should be able to cycle more rapidly. Changes in this PR are one case in point: the old clib was inconsistent, so specialized calls were necessary for a handful of methods that didn't fit the norm.

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. It's nice to see the benefit of removing these special cases in simplifying the Matlab toolbox. Just one suggestion given the discussion on clib's API stability.

@ischoegl ischoegl requested a review from speth August 6, 2024 21:00
@speth speth merged commit 32a8d4b into Cantera:main Aug 6, 2024
49 of 50 checks passed
@ischoegl ischoegl deleted the consistent-clib branch August 6, 2024 22:41
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.

2 participants