-
-
Notifications
You must be signed in to change notification settings - Fork 388
[MATLAB] Create MLTBX toolbox file that can be published on File Exchange #1882
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
Conversation
6768386
to
529e0e5
Compare
82f5f45
to
f9ff4b1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1882 +/- ##
==========================================
- Coverage 74.92% 72.63% -2.29%
==========================================
Files 448 451 +3
Lines 55763 55885 +122
Branches 9205 9203 -2
==========================================
- Hits 41780 40594 -1186
- Misses 10881 12200 +1319
+ Partials 3102 3091 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the note in the downloadDependencies.m
file, I think this should probably wait until the setup is integrated into SCons and we figure out a release process.
opts.OutputFile = outputFile; | ||
opts.SupportedPlatforms.Win64 = true; | ||
opts.SupportedPlatforms.Glnxa64 = true; | ||
opts.SupportedPlatforms.Maci64 = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this support Apple Silicon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MATLAB will run through Rosetta 2 on Apple Silicon for releases after R2020b so even though the option is called Maci64
it should support Apple Silicon.
Earlier releases will only support Mac with Intel processors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Versions 2023b and up have native support … https://www.mathworks.com/support/requirements/apple-silicon.html
907bc96
to
6513c20
Compare
- name: Set up MATLAB | ||
uses: matlab-actions/setup-matlab@718d4320188c73c86eb94ce76b553cbf89778487 # v2.5.0 | ||
- name: Set MATLAB search paths for tests and build MLTBX toolbox | ||
uses: matlab-actions/run-command@v2 |
Check failure
Code scanning / zizmor
action is not pinned to a hash (required by blanket policy) Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this CI error by using a hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed (see comment above).
190fe92
to
32f8f53
Compare
Hi @bryanwweber @ischoegl , I have been testing my MLTBX toolbox with local copies of cantera binaries and headers. Could we build those and host them somewhere similar to the PyPI package? |
Yes, we can do that, but I'd prefer to wait until this package is ready to be built. What's the status here? You have some failing checks still |
I second @bryanwweber’s opinion. I’d prefer to have a fully implemented test suite - there is a significant regression in the 1D code (#1722) that to my knowledge has not been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @ssun30 - I only have minor comments. Other than that, I believe this is mostly ready to be merged - uploading to MATLAB File Exchange is an extra step and can be addressed at a later point.
- name: Set up MATLAB | ||
uses: matlab-actions/setup-matlab@718d4320188c73c86eb94ce76b553cbf89778487 # v2.5.0 | ||
- name: Set MATLAB search paths for tests and build MLTBX toolbox | ||
uses: matlab-actions/run-command@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address this CI error by using a hash.
Aside from Ingmar's comments, I'm very confused about how all of the open PRs interact. For example, this PR changes test files but there's also the separate testing PR. Can you clarify the order we should review these? |
the MLTBX file
and upload the artifact
for the MLTBX version
Replaced ctRoot with ctPaths, this will allow the user to set paths to the shared libraries, header files, and toolboxes more freely, instead of relying on relative paths to a fixed path for a certain conda environment.
to how paths are set
- name: Set up MATLAB | ||
uses: matlab-actions/setup-matlab@718d4320188c73c86eb94ce76b553cbf89778487 # v2.5.0 | ||
- name: Set MATLAB search paths for tests and build MLTBX toolbox | ||
uses: matlab-actions/run-command@v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed (see comment above).
On a related note, I noticed that some of the same changes introduced here are also applied in #1911 (which I just reviewed). From a review perspective, it would be preferable to have single-purpose PRs so review comments can be properly tracked and addressed. |
@ssun30 (and @rwest) ... looking over this PR again, I strongly believe that this aspect should be moved to a separate packaging repository, similar to Cantera/pypi-packages or conda-forge/cantera-feedstock. This will provide some flexibility for anything that is purely related to packaging and releasing. From that perspective, I believe this PR should be closed. |
Changes proposed in this pull request
If applicable, fill in the issue number this pull request is fixing
This addresses Task 6 from this enhancement proposal: Cantera/enhancements#226
If applicable, provide an example illustrating new features this pull request is introducing
data
,interfaces/matlab_experimental
,test\matlab_experimental
,samples\matlab_experimental
into the toolbox file.Checklist
scons build
&scons test
) and unit tests address code coverage