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.

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

Move to nanmean instead of mean #211

Merged
merged 4 commits into from
Jul 7, 2017
Merged

Move to nanmean instead of mean #211

merged 4 commits into from
Jul 7, 2017

Conversation

mrkaiser
Copy link
Contributor

@mrkaiser mrkaiser commented Jun 29, 2017

Moving to nanmean allows the use of scorers that return nan without breaking existing functionality

Description

Moved the means to np.nanmean to support scorers that may implement returning

Related issues or pull requests

Link related issues/pull requests here

Pull Request requirements

  • Added appropriate unit test functions in the ./mlxtend/*/tests directories
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./mlxtend --with-coverage
  • Checked for style issues by running flake8 ./mlxtend
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
  • Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

Moving to nanmean allows the use of scorers that return nan without breaking existing functionality
@pep8speaks
Copy link

pep8speaks commented Jun 29, 2017

Hello @mrkaiser! Thanks for updating the PR.

Line 359:61: W291 trailing whitespace

Comment last updated on July 07, 2017 at 14:24 Hours UTC

@mrkaiser
Copy link
Contributor Author

The build is failing due to an issue with sudo apt-get update

@rasbt
Copy link
Owner

rasbt commented Jun 30, 2017

Thanks for the PR, and sorry about the issues with travis-ci -- I never had any issues with that and it worked up to last weekend. Hm, maybe they changed sth this week. I will look into that and let you know when it's resolved.

@rasbt
Copy link
Owner

rasbt commented Jun 30, 2017

Not sure why this issue occurred. I send in a PR for testing and it worked fine. Maybe, it was a temporary issue with the "trusty" environments on their side -- they changed something last week according to their blog. In any case, I removed the trusty flag (I think it was needed back then for the tensorflow code), and it should work fine now. Could you try again? (maybe with a tiny edit like a whitespace or sth like that and change it back).

PS: The PR looks great so far, thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 89.218% when pulling cea6299 on mrkaiser:nanmean into a457882 on rasbt:master.

@mrkaiser
Copy link
Contributor Author

mrkaiser commented Jul 7, 2017

@rasbt I have updated the builds and all checks are passing

@rasbt
Copy link
Owner

rasbt commented Jul 7, 2017

That's awesome, thanks for the PR!

@rasbt rasbt merged commit d1fdeea into rasbt:master Jul 7, 2017
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.

None yet

4 participants