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.

Sign up for GitHub

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

Jump to bottom

Remove legacy MATLAB toolbox #1670

Merged
merged 9 commits into from
Mar 28, 2024
Merged

Remove legacy MATLAB toolbox #1670

merged 9 commits into from
Mar 28, 2024

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 21, 2024 •
edited
Loading

Changes proposed in this pull request

This PR seeks to map out the removal of the legacy MATLAB toolbox

  • Remove interfaces/matlab
  • Remove documentation of legacy toolbox
  • Remove tests and samples
  • Remove MATLAB installation from build system
  • Remove references to MATLAB toolbox from new sphinx web theme

Items to be deferred:

  • Remove clib functions that were only used by legacy MATLAB toolbox
  • Bump development version number as this is a significant change

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

Closes Cantera/enhancements#196

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

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 force-pushed the remove-legacy-matlab branch from c4c2243 to ba5b9f7 Compare February 21, 2024 01:32
@ischoegl ischoegl mentioned this pull request Feb 21, 2024
Closed
@ischoegl ischoegl force-pushed the remove-legacy-matlab branch 2 times, most recently from 81cf9ee to 79becb6 Compare March 13, 2024 15:57
@ischoegl ischoegl marked this pull request as ready for review March 13, 2024 17:04
@ischoegl
Copy link
Member Author

This is ready for a review.

@ischoegl ischoegl requested a review from a team March 13, 2024 17:06
@ischoegl ischoegl mentioned this pull request Mar 13, 2024
Closed
@ischoegl ischoegl mentioned this pull request Mar 27, 2024
Closed
speth
speth requested changes Mar 27, 2024
View reviewed changes
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.

This looks good to me, with just a couple of minor changes.

include/cantera/base/logger.h Outdated Show resolved Hide resolved
data/lithium_ion_battery.yaml Outdated Show resolved Hide resolved
doc/sphinx/install/macos.md
Comment on lines -1 to -6
(sec-install-macos)=
# macOS Matlab Toolbox

The Cantera Matlab toolbox can be installed using a macOS-specific installer. The
toolbox requires macOS version 10.15 (Catalina) or higher and a copy of Matlab built for
Intel processors (which will run under Rosetta 2 on ARM-based processors).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this redirect users to the 3.0 package, as you've done for the conda and Windows installer pages?

Copy link
Member Author

@ischoegl ischoegl Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the macOS install instructions are specific to the legacy MATLAB toolbox which this PR removes, it in effect eliminates the need for the installer (which would be one less item to maintain) - from that perspective, the macOS installation instructions are moot? (the only instance ofsec-install-macos I found was in connection with MATLAB).

I did, on the other hand, add an admonition on the main install page that the legacy Matlab toolbox was removed, which would be my preferred resolution. Please let me know in case there's another use of the macOS installer that I am unaware of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the macOS package was only for the Matlab toolbox. From the standpoint of a Mac user wanting to install the Matlab toolbox, though, they need to see the directions to use the 3.0 toolbox somewhere. I think putting that information on the installation index page satisfies that scenario.

@ischoegl ischoegl force-pushed the remove-legacy-matlab branch from 79becb6 to 111e5e4 Compare March 28, 2024 05:00
@ischoegl ischoegl mentioned this pull request Mar 28, 2024
Merged
5 tasks
@ischoegl
Copy link
Member Author

ischoegl commented Mar 28, 2024 •
edited
Loading

@speth … thanks for the review. The main remaining question is about the fate of the macOS installer. Based on my understanding it’s no longer needed, and I added one more admonition to clarify.

PS: found a typo and had to force-push, but it should all be taken care of now

@ischoegl ischoegl requested a review from speth March 28, 2024 05:37
@ischoegl ischoegl force-pushed the remove-legacy-matlab branch from 111e5e4 to d740f1b Compare March 28, 2024 05:55
@speth speth merged commit eb8cde6 into Cantera:main Mar 28, 2024
50 checks passed
This was referenced Mar 30, 2024
Merged
Closed
Closed
@ischoegl ischoegl mentioned this pull request Mar 31, 2024
Closed
@bryanwweber bryanwweber mentioned this pull request Jul 18, 2024
Closed
@ischoegl ischoegl mentioned this pull request Jul 18, 2024
Closed
This was referenced Jul 27, 2024
Closed
Merged
Closed
Open
@ischoegl ischoegl mentioned this pull request Aug 9, 2024
Closed
ischoegl added a commit to ischoegl/cantera that referenced this pull request Aug 9, 2024
@ischoegl
[clib] Mark clib as experimental
df7bf3c 
After the removal of the legacy MATLAB toolbox in Cantera#1670, all remaining
interfaces that use clib are either experimental (matlab_experimental)
or rudimentary/incomplete (Fortran, .NET).

The change allows for a faster development cycle to fix accumulated
issues and inconsistencies until stable APIs have been implemented.
@ischoegl ischoegl mentioned this pull request Aug 9, 2024
Merged
5 tasks
ischoegl added a commit to ischoegl/cantera that referenced this pull request Aug 9, 2024
@ischoegl
[clib] Mark clib as experimental
05da157 
After the removal of the legacy MATLAB toolbox in Cantera#1670, all remaining
interfaces that use clib are either experimental (matlab_experimental)
or rudimentary/incomplete (Fortran, .NET).

The change allows for a faster development cycle to fix accumulated
issues and inconsistencies until stable APIs have been implemented.
ischoegl added a commit to ischoegl/cantera that referenced this pull request Aug 9, 2024
@ischoegl
[clib] Mark clib as experimental
9ed51b8 
After the removal of the legacy MATLAB toolbox in Cantera#1670, all remaining
interfaces that use clib are either experimental (matlab_experimental)
or rudimentary/incomplete (Fortran, .NET).

The change allows for a faster development cycle to fix accumulated
issues and inconsistencies until stable APIs have been implemented.
ischoegl added a commit that referenced this pull request Aug 10, 2024
@ischoegl
[clib] Mark clib as experimental
a261898 
After the removal of the legacy MATLAB toolbox in #1670, all remaining
interfaces that use clib are either experimental (matlab_experimental)
or rudimentary/incomplete (Fortran, .NET).

The change allows for a faster development cycle to fix accumulated
issues and inconsistencies until stable APIs have been implemented.
@ischoegl ischoegl deleted the remove-legacy-matlab branch August 19, 2024 12:48
pjsingal pushed a commit to pjsingal/cantera that referenced this pull request Sep 4, 2024
@ischoegl @pjsingal
[clib] Mark clib as experimental
87f01cd 
After the removal of the legacy MATLAB toolbox in Cantera#1670, all remaining
interfaces that use clib are either experimental (matlab_experimental)
or rudimentary/incomplete (Fortran, .NET).

The change allows for a faster development cycle to fix accumulated
issues and inconsistencies until stable APIs have been implemented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@speth speth speth approved these changes

Assignees
No one assigned
Labels
Matlab
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate (or remove?) legacy Matlab Toolbox in Cantera 3.1
2 participants
@ischoegl @speth