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.

Sign up for GitHub

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

Jump to bottom

DF2LibFFM #605

Merged
merged 19 commits into from
Mar 11, 2019
Merged

DF2LibFFM #605

merged 19 commits into from
Mar 11, 2019

Conversation

yueguoguo
Copy link
Collaborator

Description

A generic function that converts Pandas DataFrame to LibFFM format.

Related Issues

#604

Checklist:

  • My code follows the code style of this project, as detailed in our contribution guidelines.
  • I have added tests.
  • I have updated the documentation accordingly.

@yueguoguo yueguoguo changed the base branch from staging to ye March 5, 2019 07:59
@yueguoguo
Copy link
Collaborator Author

yueguoguo commented Mar 5, 2019 •
edited

@yexing99 this is complementary to your ongoing work for the libffm converter. As discussed offline, personally I think it might make sense to decouple a generic converter with the data-specific wrapper for "Single Responsibility" reason.

NOTE I did not branch the PR from yours. Instead, I did it from staging because we got to know that your branch has not yet updated with the staging. But in case there is anything do let me know.

miguelgfierro
miguelgfierro approved these changes Mar 5, 2019
View reviewed changes
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

LGTM

reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
gramhagen
gramhagen reviewed Mar 5, 2019
View reviewed changes
Copy link
Collaborator

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

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

this will be handy! some questions and suggestions though

reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
tests/unit/test_pandas_df_utils.py Outdated Show resolved Hide resolved
loomlike
loomlike requested changes Mar 6, 2019
View reviewed changes
Copy link
Collaborator

@loomlike loomlike left a comment

Choose a reason for hiding this comment

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

Nice to have this util! Have few comments along with some likes to other reviewers' comment!
Especially, take a look at creating the string-feature dictionary part. Regarding this, I think you should also include the test case where some samples have the same string features. Currently all the features are unique across the samples.

reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
loomlike
loomlike approved these changes Mar 7, 2019
View reviewed changes
reco_utils/dataset/pandas_df_utils.py Outdated Show resolved Hide resolved
@yueguoguo yueguoguo mentioned this pull request Mar 7, 2019
Merged
3 tasks
@yueguoguo yueguoguo changed the base branch from ye to staging March 7, 2019 14:36
@yueguoguo
Copy link
Collaborator Author

Talked with @yexing99 and base the PR on the staging branch.

@miguelgfierro miguelgfierro self-requested a review March 8, 2019 11:48
miguelgfierro
miguelgfierro requested changes Mar 8, 2019
View reviewed changes
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

As requested by @yueguoguo, stopping this for merging while we are merging staging to master #623

@yueguoguo yueguoguo merged commit d5cfb25 into staging Mar 11, 2019
@yueguoguo yueguoguo deleted the le_csv2ffm branch March 11, 2019 05:42
@yueguoguo yueguoguo mentioned this pull request Mar 12, 2019
Closed
@yueguoguo yueguoguo mentioned this pull request Mar 19, 2019
Merged
3 tasks
yueguoguo added a commit that referenced this pull request Sep 9, 2019
@yueguoguo
Merge pull request #605 from Microsoft/le_csv2ffm
9a8d52e 
DF2LibFFM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gramhagen gramhagen gramhagen left review comments

@anargyri anargyri anargyri left review comments

@miguelgfierro miguelgfierro miguelgfierro approved these changes

@loomlike loomlike loomlike approved these changes

@yexing99 yexing99 Awaiting requested review from yexing99

@Leavingseason Leavingseason Awaiting requested review from Leavingseason

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Deep Learning + Knowledge Graph Based...
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yueguoguo @miguelgfierro @gramhagen @anargyri @yexing99 @loomlike