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 problems in ThermoPhase and Kinetics classes for the experimental Matlab toolbox #1586

Merged
merged 10 commits into from
Oct 18, 2023

Conversation

ssun30
Copy link
Contributor

@ssun30 ssun30 commented Aug 11, 2023

Changes proposed in this pull request

  • Fix known problems in ThermoPhase and Kinetics classes discovered by the new test suite.
  • Remove filtered tests when problems are fixed.

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

Partially address Cantera/enhancements#177

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

PR #1496 introduced a test suite to the experimental Matlab interfaces. Currently, 12 out of 26 tests implemented in the test suite for the experimental Matlab interface are marked as filtered since they could not pass. This PR will address these problems so they will pass. Some changes to the Clib may be required as some of these problems are not related to the Matlab side of the interface.

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 Aug 12, 2023

Codecov Report

Merging #1586 (e213360) into main (de77fa3) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1586      +/-   ##
==========================================
- Coverage   72.72%   72.70%   -0.02%     
==========================================
  Files         370      370              
  Lines       56286    56298      +12     
  Branches    20369    20375       +6     
==========================================
  Hits        40932    40932              
- Misses      12357    12369      +12     
  Partials     2997     2997              
Files Coverage Δ
src/clib/ct.cpp 18.87% <0.00%> (-0.23%) ⬇️

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

@ssun30 ssun30 force-pushed the matlab_patches branch 2 times, most recently from 1f3b9f7 to 5d72afe Compare September 26, 2023 22:42
@ssun30
Copy link
Contributor Author

ssun30 commented Sep 27, 2023

Sorry I've been busy with class and proposal defense in the past few weeks. I just got some time to go over the PR again. I mentioned to Ray earlier that some methods could not return correct error messages. It turns out these are methods that return certain indices such as thermo_speciesIndex and thermo_elementIndex.

They do not throw exceptions when the returned values are out of bounds (npos). Instead, there are dedicated methods to check whether these indices are out of bounds, for example Phase::checkSpeciesIndex. From my understanding, it was done this way so that three-body reactions (with undefined species 'M') do not cause errors when they are imported. Therefore I've moved the error handling into the Clib layer.

The other remaining issue with the ThermoPhase class in the Matlab interface is that multi-property setters (such as ThermoPhase.TDY may set incorrect values. I couldn't pinpoint the cause just yet and will test it on Thursday.

src/clib/ct.cpp Outdated Show resolved Hide resolved
@ssun30 ssun30 marked this pull request as ready for review October 3, 2023 21:28
@ssun30 ssun30 force-pushed the matlab_patches branch 2 times, most recently from 1acdc3f to be01c64 Compare October 3, 2023 21:35
@ssun30 ssun30 requested a review from speth October 4, 2023 21:05
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 these updates, @ssun30. I think this is important progress for the new Matlab toolbox, and I'm glad to see it moving forward. I had just a few suggestions related to the current updates.

interfaces/matlab_experimental/Base/Kinetics.m Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Base/Kinetics.m Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Base/ThermoPhase.m Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Base/ThermoPhase.m Outdated Show resolved Hide resolved
interfaces/matlab_experimental/Base/ThermoPhase.m Outdated Show resolved Hide resolved
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, @ssun30! This looks good to me.

@speth speth merged commit 3a9bc6a into Cantera:main Oct 18, 2023
38 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants