-
Notifications
You must be signed in to change notification settings - Fork 8
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
Dataframes and rules in tsv; OpenIE; Rule, Ruleset formats; READMEs; Unit tests #40
Conversation
Just a suggestion on how to write good commit messages: https://cbea.ms/git-commit/ |
Do you still support json or is it only tsv now? |
if path.endswith(".json"): | ||
rule_set.to_json(path) | ||
elif path.endswith(".tsv"): | ||
rule_set.to_tsv(path) | ||
else: | ||
raise ValueError("Unknown file extension, currently only .json and .tsv are supported") | ||
raise ValueError( |
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.
Add black as commit hook? :P
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.
Yes, will do that, thanks :)
["(u_0 / find :obl (u_1 / .*) :obj (u_2 / .*))"], | ||
[], | ||
"FIND", | ||
[{"ARG1": 1, "ARG2": 2}], |
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.
What are these?
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.
@Eszti New arguments for OpenIE, you can find examples in https://github.com/adaamko/POTATO/blob/dev/notebooks/openie.ipynb
if path: | ||
self._dataset = self.read_dataset(path=path, binary=binary) | ||
else: | ||
self._dataset = self.read_dataset(examples=examples) | ||
self.extractor = GraphExtractor(lang=lang, cache_fn=f"{lang}_nlp_cache") |
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.
I suggest you leave an option for the cache_fn to be configured from outside
] | ||
if len(g) > 1: | ||
print( | ||
"WARNING: graph has multiple connected components, taking the largest" |
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.
what about Kinga's solution for representing multiple connected components in penman? Is it too soon to use that?
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.
I think currently we don't have a good solution for the issue, but correct me if I'm wrong @GKingA
else: | ||
raise ValueError("No examples or path provided") | ||
|
||
# ADAM: THIS WILL NEED TO BE ADDRESSED |
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.
what is this about? Maybe open an issue and reference it in the comment
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.
Did in #41
@Eszti Thank you for the suggestion, I will also check it :) |
close #39
close #26