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

Handling pandas.core.series.Series in matrix_utils.py #1700

Merged
merged 1 commit into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@bhavya01
Copy link
Contributor

commented Feb 1, 2019

Addresses #1694

@mlpack-bot

This comment has been minimized.

Copy link

commented Feb 1, 2019

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

@harshitbansal05
Copy link

left a comment

Great, LGTM.

@mlpack-bot

This comment has been minimized.

Copy link

commented Feb 1, 2019

Hello there! Thanks for your contribution. I see that this is your first contribution to mlpack. If you'd like to add your name to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt and you haven't already, please feel free to push a change to this PR---or, if it gets merged before you can, feel free to open another PR.

In addition, if you'd like some stickers to put on your laptop, I'd be happy to help get them in the mail for you. Just send an email with your physical mailing address to stickers@mlpack.org, and then one of the mlpack maintainers will put some stickers in an envelope for you. It may take a few weeks to get them, depending on your location. 👍

@ShikharJ

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Hmm, looks like mlpack-bot passes on this message on any approval.

@rcurtin

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

mlpack-bot also didn't wait 24 hours like it was supposed to. :( I guess I have two bugs to fix.

Anyway thanks for the PR @bhavya01---I'll review it shortly.

@rcurtin

rcurtin approved these changes Feb 4, 2019

Copy link
Member

left a comment

This fixes the segfault and I think is a reasonable way to solve the problem, but in reviewing this and looking at the code, I realized that we are copying the data perhaps unnecessarily when Pandas dataframes are used. So I think we should merge this for now, but I'll open another issue about the unnecessary copying and that can be handled later.

Thanks for the contribution! 👍

@mlpack-bot

mlpack-bot bot approved these changes Feb 5, 2019

Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit d6910b1 into mlpack:master Feb 6, 2019

6 checks passed

LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Thanks again for the contribution! 👍 mlpack-bot was supposed to wait until now to offer you stickers, but anyway if you want them just email stickers@mlpack.org. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.