-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Matlab toolbox revamp #1182
Matlab toolbox revamp #1182
Conversation
@ssun30 I converted this to a "draft" pull request since you put WIP in the title. If that wasn't correct, please feel free to click the "Ready for review" button near the bottom of the page (right above the "Checks" box) whenever you want us to take a detailed look at this 😄 |
Linking #379 |
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.
Hi @ssun30 ... great to see this moving along! While this is still WIP, I decided to leave a brief comment as I'm not sure that the automatic figure generation should make it into the updated toolbox.
Beyond, there are missing empty lines at the end of many files, as well as some spurious Windows line-breaks (shown below as ^M
) that you're likely already aware of. There likely is a setting in your code editor that can take care of these minor formatting issues. I'm definitely looking forward to seeing the final PR!
PS: I’m also not certain that the XML_node.m
needs to be implemented as XML/CTI is completely replaced by YAML, with the former being discontinued after the 2.6 release.
I think this is now ready for reviewers' eyes? |
@ssun30 Please see this comment from @ischoegl above and address those comments before we move further with the review! Thanks 😄 |
These should be resolved now. |
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.
@ssun30 … thanks for addressing!
could you please fix the commits pertaining to libexecstream
(this is an external package; it looks like you accidentally added/changed something there)?
It doesn't seem I changed these files in libexecstream and data/gri30_highT.cti. What's really weird is whenever I commit some changes, I see these specific files are modified even though I didn't do anything to them. I want to fix the commit but I'm running into a consistent problem with letter capitalization that I couldn't recall how I resolved last time. I'll ask Chris who knows the solution. |
e4711e9
to
c7a6f18
Compare
Fixed! |
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.
@ssun30 Thanks for all your work on this so far! I've left a few comments here that I think will apply to the entire set of changes. I didn't get a chance to look through all the files yet. I'll try to do some more over time, but you should get started on the changes I've got here now and keep updating the PR. Thanks!
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.
@ssun30 ... like @bryanwweber I appreciate your continued efforts on this PR!
I have two comments:
- Could you update your toolbox path?
interfaces/Matlab_Toolbox_Revamp
is ok for work-in-progress, but it probably should be renamed - More importantly, one of my main questions is about how you have been testing the toolbox thus far - do you have any test files already implemented?
Regarding my second question, I am asking as much of Cantera is covered by extensive unit tests. Since the previous MATLAB toolbox was implemented (which has some rudimentary tests), Mathworks has come up with much better testing frameworks, see here. While I have not checked into this, Mathworks also supports runners on GitHub, see run-matlab-tests action. Overall, from a maintenance perspective, it would be great if you could add some basic testing. It doesn't have to be highly detailed, but even a relatively basic test suite would go a long way.
As an aside, there are many open issues for the existing MATLAB toolbox, see list of open MATLAB issues. It would be great if you could reference those that can be closed based on your ongoing work.
Thanks @ischoegl! @ssun30 I forgot to mention this point in my comment earlier. I'm having a hard time seeing how the new interface will work. Can you update a couple of the old samples to use the new syntax? It doesn't have to be all of them at the moment, but 2-3 would be great to see how the new interface compares to the other. As far as adding some tests, the existing tests are here: https://github.com/Cantera/cantera/tree/main/test/matlab You can run them locally by doing |
I thought you had rewritten all the examples @ssun30. Are they not in this PR? |
Thanks @bryanwweber … regarding the old unit tests, I believe it will make sense to move away from matlab_xunit (or at least upgrade), as it is 10 years old / several versions behind, and is not necessarily recommended, see comment here. |
From what I can tell (caveat: I don’t have MATLAB installed), samples in the On a different note regarding deprecations, it probably makes sense to eliminate everything that is already deprecated elsewhere in the upcoming Cantera 2.6. |
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.
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.
I wanted to bring up one additional point, which was originally raised by @decaluwe in Cantera/enhancements#102, specifically here: as there already are drastic/breaking changes in the MATLAB toolbox syntax, the following should be given some thought:
- Avoid method names where MATLAB follows a convention different from other API's. Example:
ThermoPhase.atomicMasses
(proposed MATLAB) vs.ThermoPhase.atomic_weights
(Python); C++ uses scalaratomicWeight
getters. - There is the option to make things completely consistent with the Python API by introducing
snake_case
property/method names. I pointed out a couple of instances, but this would apply to the entire toolbox. - Beyond, both Python and MATLAB support methods and properties for classes. It would be nice if both API’s would use a consistent approach.
Beyond, as in the 'old' MATLAB toolbox there are still substantial gaps of functionality compared to the Python API. This is to a certain extent a function of an incomplete C API, but should be addressed eventually.
Personally, I think each interface should represent idiomatic code in that language. As such, in my opinion, the Matlab interface should use |
…tion guide. Removed most nested IF statements.
constructors instead.
ThermoPhase methods.
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.
I pushed an update to eliminate a lot of unnecessary "churn" from the commit history, as well as fixing end-of-line whitespace and a number of typos.
I think we can discuss the next steps in the Enhancements repo, either as part of Cantera/enhancements#102 or a new topic.
Wow. Thank you all! |
Tagging Cantera/enhancements#177 |
Changes proposed in this pull request
If applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#32
Checklist
scons build
&scons test
) and unit tests address code coverage