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

add a ACEReaderWithTrueCaseFixer class #581

Merged
merged 6 commits into from
Nov 2, 2017
Merged

Conversation

Slash0BZ
Copy link
Member

@Slash0BZ Slash0BZ commented Nov 1, 2017

update usages upon this new class
Also a bug fix that cases opposite direction relations in RelationAnnotator.
Also added two new testing functions in MD and RE.

@Slash0BZ
Copy link
Member Author

Slash0BZ commented Nov 1, 2017

closes #580

@danyaljj
Copy link
Member

danyaljj commented Nov 1, 2017

I was thinking that it might be a good idea if we move ACEReaderWithTrueCaseFixer.java (which is in corpus readers) to external/stanford_3.3.1 (hence we don't have to add stanford as a dependency to corpus-readers). What do you think? (@mssammon feel free to comment on this)

@Slash0BZ
Copy link
Member Author

Slash0BZ commented Nov 1, 2017

Ah that way ACEReaderWithTrueCaseFixer will be a package in stanford_3.3.1 so it can't extend ACEReader anymore right?

@mssammon
Copy link
Contributor

mssammon commented Nov 1, 2017

the new reader can be in the external/ package, I agree... maybe point to it in the corpusreader documentation, though. I don't see why the reader can't extend ACEReader; package name/path doesn't have to be identical.

@danyaljj
Copy link
Member

danyaljj commented Nov 1, 2017

So rather than copus-readers being dependant on stanford, stanford would be dependant on stanford.

@Slash0BZ
Copy link
Member Author

Slash0BZ commented Nov 1, 2017

Just making sure, are we moving it to edu.illinois.cs.cogcomp.pipeline.common which holds all the external sources?

This way, external/stanford_3.3.1 will depends on corpusreader and it's fine right? @danyaljj

@danyaljj
Copy link
Member

danyaljj commented Nov 1, 2017

@Slash0BZ
Copy link
Member Author

Slash0BZ commented Nov 1, 2017

I understand, but all files in stanford_3.3.1 are in package edu.illinois.cs.cogcomp.pipeline.common.

@danyaljj
Copy link
Member

danyaljj commented Nov 1, 2017

Ah you can keep the package naming compatible with corpus reader (which is what you currently have).

<dependency>
<groupId>edu.illinois.cs.cogcomp</groupId>
<artifactId>illinois-corpusreaders</artifactId>
<version>3.1.35</version>
Copy link
Member

Choose a reason for hiding this comment

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

3.1.36

@danyaljj
Copy link
Member

danyaljj commented Nov 1, 2017

Minor comment on versioning: merge after fixing it (and CI turning green).

@danyaljj danyaljj merged commit 9da99c0 into CogComp:master Nov 2, 2017
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