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

fix the timex bug when may and sat appear as verbs instead of timexes #663

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

qiangning
Copy link
Member

@qiangning qiangning commented Jun 14, 2018

Previously, to avoid silly mistakes, I incorporated rules before timex chunker, so that any word matching to month names or week-of-the-day names will be forced to be a timex.

However, I realized that this rule is not accurate. may can be either a modal verb, or the month. sat can be also the past tence of sit. So I have added a POS check now: only those may's or sat's that are NNP will be forced to be timex.

sun is a bit tricky, since the star Sun is also NNP. I pick it out of our rules and leave it up to the chunker's decision.

@qiangning
Copy link
Member Author

Daniel @danyaljj, Mark @mssammon : I suggest Zhili @666666fzl review this change.

@danyaljj danyaljj requested a review from powpos360 July 24, 2018 16:55
Copy link
Contributor

@powpos360 powpos360 left a comment

Choose a reason for hiding this comment

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

LGTM

@qiangning qiangning merged commit 4894fbc into CogComp:master Jul 24, 2018
@qiangning
Copy link
Member Author

@danyaljj I've merged this PR. Since this is my first time merging PRs, are there anything else I need to do after the merge?

@danyaljj
Copy link
Member

Nope. You're good.

Note that the change is not deployed yet. In other words, the jar files in the maven repo (http://cogcomp.org/m2repo/edu/illinois/cs/cogcomp/) still don't contain your changes. We deploy only when there is a need for doing so. If you need it, you have to deploy it (details here)

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.

3 participants