-
Notifications
You must be signed in to change notification settings - Fork 116
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
Mpl2 display #234
Mpl2 display #234
Conversation
Note: this PR needs to get merged before any of the others, because display tests are currently breaking everything else. |
It looks like the source separation metrics are getting some minor errors now. I can't tell if it's due to numerical precision or an actual change in the data. Note: i didn't change any data here, only locations. @dawenl got a minute to investigate? |
I suspect that issue is orthogonal to this PR though. EDIT: confirmed that these separation failures are independent of the changes in this PR. I recommend we make a separate PR to fix the separation fixtures. |
@faroit or @ecmjohnson may be the best people to ask since they touched the codebase most recently. |
@craffel I will have look this weekend |
As mentioned over in #236, I think this one's good to go. The only remaining failures are on separation metrics. A quick CR would be nice, since this PR does change a little of how the tests are executed (beyond just the display submodule). It does not change any functionality though. |
|
||
before_script: | ||
- pep8 mir_eval evaluators tests | ||
|
||
script: | ||
- nosetests -v --with-coverage --cover-package=mir_eval | ||
- nosetests -v --with-coverage --cover-package=mir_eval -w tests |
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.
So the -w tests
tells nosetests
it's running within the tests
folder, and this is why you removed the tests
prefix from all the paths?
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.
Yep. This way you can also execute from within the tests
directory, and the tests don't have to know that they live within the tests/
directory of the repo.
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.
Cool.
Looked it over, looks good. Just want to reiterate how awesome the display module is!! |
@craffel thanks! merge away? |
@faroit did you have a chance to look at the source sep fixtures? |
This PR updates the test suite for matplotlib 2.0 compatibility, and fixes #233 .
Note: the display still works on 1.5, but for testing purposes, we now use 2.0 (and 2.0 default styles).
I had to change a couple of small things in the test setup to get things working -- notably, removing
tests/__init__.py
(tests does not need to be a module), and added an extras_require for testing.