Conversation
|
Thanks for working on this! I just wanted to let you know that I will look On Mon, Jul 28, 2014 at 1:42 PM, Ismail Badawi notifications@github.com
|
|
Okay, thanks! I ran into still more issues with jsonlab, including a couple of tests that I couldn't get to pass. I gave up on it and just modified the existing json package. It was way less work than I thought it would be (see the first two commits), and everything seems to work well. I should have just done that from the beginning. I rebased and |
* Extract most of the Matlab class into a base Session class * Matlab and Octave are subclasses that tweak the command line invocation * Eliminate some code duplication along the way
There was a problem hiding this comment.
The maxtime parameter wasn't actually being used in run_func, run_code or get_variable, so I removed it and then fixed the call sites. I guess we could implement a timeout in those methods, but even then it might make more sense to just use self.maxtime rather than a parameter.
|
Looks good - tests are all passing on my setup. Would you mind writing some tests for Octave? I don't have it currently installed, but will install and try those out, if you write them. Other than that, I only had a couple of comments. Let me know when you want me to take another look, and merge this in. |
|
Okay, sure. Rather than writing new tests, I made it so that the existing tests will run under Octave if you set the Finally, in order to run the tests you need to compile the messenger for Octave, as I mentioned initially. I can do this with an invocation like |
|
Great. Maybe just add a bit to the README file to describe these differences, and tell people what to do under Octave? Apart from that, I think this is ready to be merged. Thanks a lot for making this pull request! |
|
Okay, I added a bit to the README. I just realized that renaming the |
|
I think that's OK - it's a relatively small change, which would be easy for people to fix up in their code. I will merge this, and we can see if we start getting a lot of complaints about this changed... |
A new "backend" setting can be set to either "MATLAB" or "Octave"; this affects both the shell and the call graph instrumentation. This depends on arokem/python-matlab-bridge#75. A sort-of-unrelated change in this commit is that the Matlab or Octave session is initialized asynchronously after the page has loaded, instead of making the page hang.
2 participants
This is a followup to #74. I don't expect this to be merged yet, but I'd like to get feedback on a few things.
I got this mostly working with Octave. I did a few things:
jsonpackage being used and replaced it withjsonlab. It's pure MATLAB -- I'm not sure what the performance implications of this are. I've also run into weird issues around special characters. The interface at least is similar;loadjsoninstead ofjson.load, andsavejsoninstead ofjson.dump. With the issues I ran into I'm not sure about how good this library is -- an alternative might be to make changes to the original json package to be compatible with Octave (remove the +pkg style, and rewrite the code that interacts with the java classes). Anyway, this is the first commit.pymatbridge.pyto handle the different command line interfaces for Matlab and Octave. I extracted most of theMatlabclass into a baseSessionclass, withMatlabandOctaveas subclasses. Along the way I eliminated some small bits of code duplication. This is the second commit.mkoctfile --mex. (No C code changes required). I'm not sure how to integrate this with the build system and prebuilt binaries you have here.Finally, there didn't seem to be a way to disable showing plots in Octave, like
-nodisplaydoes for Matlab, short of runningfigure('visible', 'off')before every command. I'm not sure what to do about this.Let me know what you think of this so far.