Skip to content

Conversation

jjangsangy
Copy link

Continuation of #172

Changes

  • Only relevant commits to messenger package are included.
  • Docstrings revised to fit scipy spec
  • Use platform.machine().endswith('64') over sys.maxsize
  • Added unit tests to messenger/tests
  • Tests ensure Matlab architecture is the same as compiled messenger binary.

-------
Namespace containing parsed arguments
"""
parser = ArgumentParser(prog='messenger')
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be 'build-messenger'? It might be confusing that this is called 'messenger'

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, should we rename the entire module to be build-messenger, or just this section of the code?

Decided to start implementing the big merge with setuptools over current tools consolidated in the messenger module.
 Hooking into setuptools allows for automatic discovery of requirements and the ability to fine tune almost every aspects of the build process.
import nose
args = 'nosetests -v --exe --with-cov '
if sys.version_info == (2, 7):
args += '--cover-package pymatbridge'
Copy link
Owner

Choose a reason for hiding this comment

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

Coverage report should not be mandatory on some versions. Instead it should be an option. That is

python setup.py test --with-coverage 

should achieve this effect. Is that possible to do with this?

Notice also that the coverage report on travis seems to be doing something wrong. We are getting a coverage report on some elements of IPython, instead of pymatbridge. Could you take a look at that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a .coveragerc, see the one from scikit-image

@arokem
Copy link
Owner

arokem commented Apr 24, 2015

A couple of comments:

  • I tried installing this using pip from github on a separate env. It seems to install fine and import finem but I am getting Undefined function 'matlabserver' for input arguments of type 'char'.
  • The tests pass, but I think that's because they don't get installed properly. Coverage on the pymatbridge module is down to 19%.

@jjangsangy
Copy link
Author

So after scratching my head for an hour or so, I think I might have figured it out.

  1. My MANIFEST.in was wrong. I don't know how much of this may have contributed to the issue, but it may be the reason why Undefined function 'matlabserver' for input arguments of type 'char'. error might have arisen.
  2. For the tests to run there are several dependencies I did not consider.
    • ipython[all] # We need ipython notebook for matlab_magic tests.
    • numpy
    • wheel # In order to install numpy binary

@jjangsangy
Copy link
Author

Okay after making these changes and running under virtualenv, heres my coverage report

Name                             Stmts   Miss  Cover   Missing
--------------------------------------------------------------
pymatbridge                         11      4    64%   7-8, 12-13
pymatbridge.__version__              3      0   100%
pymatbridge.compat                   7      2    71%   6-7
pymatbridge.matlab_magic            95      8    92%   35-37, 40, 43, 77, 102, 166-167
pymatbridge.messenger                2      0   100%
pymatbridge.messenger.make         106     69    35%   14-15, 36-87, 97, 112-125, 130-134, 148-164, 193, 199, 203
pymatbridge.messenger.settings      15      1    93%   7
pymatbridge.publish                 49      0   100%
pymatbridge.pymatbridge            233     36    85%   57-58, 80-83, 217, 228, 255-256, 287, 296, 319, 326, 334-343, 388, 396, 445, 496-498, 502, 505-509, 512, 554
--------------------------------------------------------------
TOTAL                              521    120    77%
----------------------------------------------------------------------

The complete coverage is still going down, but this is primarily due to the fact that the messenger is now incorporated into pymatbridge.

@arokem
Copy link
Owner

arokem commented Apr 25, 2015

Did you push these changes to your master branch? I'm not getting them in
a pull.

On Fri, Apr 24, 2015 at 5:36 PM, Sang Han notifications@github.com wrote:

Okay after making these changes, heres my coverage report

Name Stmts Miss Cover Missing

pymatbridge 11 4 64% 7-8, 12-13
pymatbridge.version 3 0 100%
pymatbridge.compat 7 2 71% 6-7
pymatbridge.matlab_magic 95 8 92% 35-37, 40, 43, 77, 102, 166-167
pymatbridge.messenger 2 0 100%
pymatbridge.messenger.make 106 69 35% 14-15, 36-87, 97, 112-125, 130-134, 148-164, 193, 199, 203
pymatbridge.messenger.settings 15 1 93% 7
pymatbridge.publish 49 0 100%

pymatbridge.pymatbridge 233 36 85% 57-58, 80-83, 217, 228, 255-256, 287, 296, 319, 326, 334-343, 388, 396, 445, 496-498, 502, 505-509, 512, 554

TOTAL 521 120 77%

The complete coverage is still going down, but this is primarily due to
the fact that the messenger is now incorporated into pymatbridge.


Reply to this email directly or view it on GitHub
#173 (comment)
.

@jjangsangy
Copy link
Author

Doing it now, there's some code that I had to deactivate in this build. But let's see if I can replicate the coverage report under travis!

@jjangsangy
Copy link
Author

So strange, for some reason now I'm getting error when running this under a python2.7 virtualenv

error: file '/Users/jjangsangy/Projects/python-matlab-bridge/s' does not exist

Have no idea where in the code is trying to read a file called s.

Can't seem to figure this one out. I think I'm going to sleep on this one and try to tackle it again in the morning.

@jjangsangy
Copy link
Author

Okay, now I was able to replicate the issue on Travis.

error: file '/Users/jjangsangy/Projects/python-matlab-bridge/s' does not exist

It looks like it is actually one of the build scripts running on pymatbridge side of things that seems to be causing the strange formatting issue.

This issue has become such a pain that I'm actually considering starting over from scratch from one of working builds in order to get things running again.

@arokem
Copy link
Owner

arokem commented Apr 28, 2015

I am not sure I entirely understand what "from one of working builds"
means, but if you do start over and break down this PR to several
step-by-step PRs, instead of all at once, it will be easier to review and
merge. If you don't mind breaking all this work up into stages, I think
that it will be easier for everyone.

On Tue, Apr 28, 2015 at 12:01 AM, Sang Han notifications@github.com wrote:

Okay, now I was able to replicate the issue on Travis.

error: file '/Users/jjangsangy/Projects/python-matlab-bridge/s' does not exist

It looks like it is actually one of the build scripts running on
pymatbridge side of things that seems to be causing the strange
formatting issue.

This issue has become such a pain that I'm actually considering starting
over from scratch from one of working builds in order to get things running
again.


Reply to this email directly or view it on GitHub
#173 (comment)
.

@jjangsangy
Copy link
Author

Sure, closing this PR then.

@jjangsangy jjangsangy closed this May 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants