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

Complete the experimental Matlab toolbox. #177

Open
ssun30 opened this issue May 18, 2023 · 13 comments
Open

Complete the experimental Matlab toolbox. #177

ssun30 opened this issue May 18, 2023 · 13 comments
Labels
work-in-progress An enhancement that someone is currently working on

Comments

@ssun30
Copy link

ssun30 commented May 18, 2023

Abstract

Implement the currently missing features in the experimental Matlab interface.

Motivation

Describe the need for the work being done:

  • For Ubuntu users, a prefix must be added to the command line to correctly load GLIBC into Matlab.
  • There is no tutorial. Users could only learn about the toolbox by reading the code of the new samples. The readme.md for the toolbox is not intuitive for users to follow.
  • Building documentation with Sphinx is currently not enabled for the new interface.
  • Currently the only way to test the toolbox is running the test_examples.m script which does not cover all methods in all classes.
  • Without automated installation there is no easy way to adding testing into the CI workflow.
  • Building from source is currently the only way to have access to the new interface. A pkg installer for Mac OS users and msi installer for Windows users is needed for the official release.

Description

  • For building documentations, it's sufficient (for now) to pin Sphinx version to 5.1.1 in our CI. This could be addressed in a quick PR.

References

Links to a development branch in your fork of the Cantera repository, Pull Requests, GitHub Issues, Users' Group topics, or other relevant material.

@ssun30 ssun30 added the feature-request New feature request label May 18, 2023
@speth
Copy link
Member

speth commented May 18, 2023

Without automated installation there is no easy way to adding testing into the CI workflow.

I think introducing a test suite that can be run through GitHub Actions should be the top priority for this toolbox. I'm not sure why automating installation would be necessary for this -- one of the best features of using calllib is that there is nothing to compile on the Matlab side -- all you need is a copy of cantera_shared.so and the header files.

If you can get started on the test suite itself, I'd be happy to help with the GitHub Actions integration.

@ischoegl
Copy link
Member

@ssun30 ... thanks for posting. Is this work in progress or a feature request?

@ssun30
Copy link
Author

ssun30 commented May 25, 2023

@ssun30 ... thanks for posting. Is this work in progress or a feature request?

This is a work in progress. Sorry I couldn't find where to change the label of this issue.

@ischoegl ischoegl added work-in-progress An enhancement that someone is currently working on and removed feature-request New feature request labels May 26, 2023
@ischoegl
Copy link
Member

Fantastic… changed the label for you.

@rwest
Copy link
Member

rwest commented Jun 1, 2023

We think @ssun30's new test suite has found some bugs (hurrah - it's working). Should the bug fixes be a separate pull request? How is it best structured and broken up? (We'd probably like to avoid reviewing another gazillion-line pull request.)

@ischoegl
Copy link
Member

ischoegl commented Jun 1, 2023

We think @ssun30's new test suite has found some bugs (hurrah - it's working). Should the bug fixes be a separate pull request? How is it best structured and broken up? (We'd probably like to avoid reviewing another gazillion-line pull request.)

🎉 … my 2 cents would be to create PR’s that collect those bugs, using ‘draft’ mode and the ‘ready for review’ check box to mark progress. That way, we should be able to review a small number of PR’s without getting to an excessive line count.

@speth
Copy link
Member

speth commented Jun 1, 2023

As a starting point, I'd say it would be nice to begin with a PR that introduces some portion of the test suite and a way of running it (assuming that's in place) without getting into too many bug fixes in the first PR. If there are tests that currently fail, you can mark them as such so they don't cause the whole test suite or CI to indicate failure.

From there, you can create new PRs that both introduce new test code and bug fixes needed for those tests to succeed. This doesn't need to be "one bug per PR", but it would be good to try to limit the scope of any individual PR, be that in terms of number of lines of code to review, type of bug being fixed, or number of files/functions impacted.

@ischoegl
Copy link
Member

ischoegl commented Jun 2, 2023

As a starting point, I'd say it would be nice to begin with a PR that introduces some portion of the test suite and a way of running it (assuming that's in place) without getting into too many bug fixes in the first PR. […]

… on second thought that would indeed be nice. I assume you’re using Mathworks’ testing frameworks … in which case there likely isn’t much required to make this run? Initial tests can be brutally simple. As long as the approach is compatible with the GH Matlab action or similar - which probably needs to be tested before investing too much work - simple instructions may suffice for starters.

@ischoegl
Copy link
Member

@ssun30 ... fwiw, I just refactored the Func1 functor interface (see Cantera/cantera#1513), which should make things a lot simpler going forward. Once merged, this will make all Func1 specializations accessible from clib, which should help with a resolution of Cantera/cantera#567 in the new interface.

@ischoegl
Copy link
Member

@ssun30 … thank you for all your work on this thus far. Given that we released 3.0 with the experimental version of this MATLAB toolbox, what are the remaining steps for the final release?

@ssun30
Copy link
Author

ssun30 commented Jan 23, 2024

@ssun30 … thank you for all your work on this thus far. Given that we released 3.0 with the experimental version of this MATLAB toolbox, what are the remaining steps for the final release?

Hi Ingmar, sorry I've been busy working on my dissertation proposal and class projects towards the end of last year. Now I have time to work on the toolbox again. I think the remaining tasks for the final relase include:

  • Complete the test suite and add these to the CI (In Progress)
  • Clean up the documentations.
  • Incorporate the installation of the toolbox into the SCons scripts.
  • Build the msi package for Windows.

@ischoegl
Copy link
Member

ischoegl commented Jan 23, 2024

@ssun30 … thank you for all your work on this thus far. Given that we released 3.0 with the experimental version of this MATLAB toolbox, what are the remaining steps for the final release?

Hi Ingmar, sorry I've been busy working on my dissertation proposal and class projects towards the end of last year. Now I have time to work on the toolbox again. I think the remaining tasks for the final relase include:

  • Complete the test suite and add these to the CI (In Progress)
  • Clean up the documentations.
  • Incorporate the installation of the toolbox into the SCons scripts.
  • Build the msi package for Windows.

Thank you, @ssun30! That sounds great - it appears to be reasonably close to the finishing line. Please let us know in case you should need any assistance with the finishing touches. I'm definitely looking forward to a release in a 'stable' version.

FWIW: regarding documentation, the current builds for 3.1 alpha use sphinx>=7.2,<8 (for whatever is built using GH Actions)

@ischoegl
Copy link
Member

ischoegl commented Aug 2, 2024

@ssun30 and @rwest ... do you have any updates here or for Cantera/cantera#1665? I believe @speth and @bryanwweber are pushing towards release of Cantera 3.1 relatively soon. While the legacy MATLAB toolbox version is now removed as of Cantera/cantera#1670, the experimental toolbox will remain largely unchanged from Cantera 3.0, with first known bugs (e.g. Cantera/cantera#1722)?

PS: Changes introduced in Cantera/cantera#1741, Cantera/cantera#1754 and Cantera/cantera#1760 were motivated by removals and/or updates of legacy clib code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress An enhancement that someone is currently working on
Projects
None yet
Development

No branches or pull requests

4 participants