Skip to content

Conversation

@jrwishart
Copy link

There was a bug that if a single entity is found in all inputs (documents), then the mapply function would simplify the data structure and cause an error. This has been fixed with SIMPLIFY = FALSE. Added a unit test to verify bug is fixed.

Also, passing the input file path to NERAnnotate is redundant since the input file path is set in the cnlp properties. Removed that string argument, added a check that the file parameter exists in cnlp.

There was a bug that if a single entity is found in all inputs (documents), then the mapply function would simplify the data structure and cause an error. This has been fixed with SIMPLIFY = FALSE. Added a unit test to verify bug is fixed.

Also, passing the input file path to NERAnnotate is redundant since the input file path is set in the cnlp properties. Removed that string argument, added a check that the file parameter exists in cnlp.
@jrwishart jrwishart marked this pull request as ready for review November 13, 2019 22:41
@jrwishart jrwishart requested a review from mwmclean November 13, 2019 22:43
@jrwishart
Copy link
Author

Strictly speaking this is a breaking change (incompatible API) and warrants a version bump to 3.0.0 since calls to NERAnnotate have changed.

NERAnnotate previously required an input file string path of the form NERAnnotate("path/to/input.file"). However, this string argument is removed since the file path that CoreNLP uses is already stored in the java properties in the volatiles environment.

I only incremented the patch version from 2.4.2 -> 2.4.3 and updated flipTextAnalysis DESCRIPTION to require 2.4.3 since I updated the calls to NERAnnotate there also.

@mwmclean mwmclean merged commit 288ee0f into master Nov 14, 2019
@mwmclean
Copy link

Please separate some of the tests in test-entity.R into separate test_that() blocks describing what each is testing.

@jrwishart
Copy link
Author

Please separate some of the tests in test-entity.R into separate test_that() blocks describing what each is testing.

Done in 3f3a242

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