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

reorg_lf_helpers #81

Merged
merged 3 commits into from
Jul 21, 2018
Merged

reorg_lf_helpers #81

merged 3 commits into from
Jul 21, 2018

Conversation

senwu
Copy link
Collaborator

@senwu senwu commented Jul 21, 2018

No description provided.

@senwu senwu assigned senwu and lukehsiao and unassigned senwu and lukehsiao Jul 21, 2018
@senwu senwu requested a review from lukehsiao July 21, 2018 10:35
from fonduer.supervision.utils.structural import *
from fonduer.supervision.utils.tabular import *
from fonduer.supervision.utils.textual import *
from fonduer.supervision.utils.visual import *
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way to organize this so that the imports to the user is the same is to make lf_helpers the directory with an init.py, and having the separate modaility helper files in that directory, rather than making a utils directory. If that sounds OK to you, I can take care of that update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's sound right! Thanks!

@lukehsiao lukehsiao added this to the v0.2.0 milestone Jul 21, 2018
@lukehsiao lukehsiao added the clean-up Cleaning up the code or refactoring label Jul 21, 2018
Copy link
Contributor

@lukehsiao lukehsiao left a comment

Choose a reason for hiding this comment

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

Other changes LGTM.

)


def is_superset(a, b):
Copy link
Contributor

Choose a reason for hiding this comment

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

This function and overlap() seemed too general to be in tabular.py, so I moved them here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

return


def get_visual_header_ngrams(c, axis=None):
Copy link
Contributor

@lukehsiao lukehsiao Jul 21, 2018

Choose a reason for hiding this comment

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

I don't expose these unimplemented helpers in the __init__.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@senwu senwu merged commit a46ff87 into master Jul 21, 2018
@senwu senwu deleted the reorg_lf_helpers branch July 21, 2018 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up Cleaning up the code or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants