Dev tokens by koheiw · Pull Request #887 · quanteda/quanteda

koheiw · 2017-07-27T20:27:32Z

Adds tokens.tokens(). Also gives much greater control of types of tokens to be preserved, while increasing efficiency.

…-tokens

codecov · 2017-07-27T21:02:12Z

Codecov Report

Merging #887 into master will decrease coverage by 0.03%.
The diff coverage is 84.9%.

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
- Coverage   77.72%   77.68%   -0.04%     
==========================================
  Files         106      107       +1     
  Lines        7976     8142     +166     
==========================================
+ Hits         6199     6325     +126     
- Misses       1777     1817      +40

kbenoit

Looks good.

Most of my comments are for consideration, not for changes, but do take a look at the remove_hyphens behaviour.

For the next version, let's make it a priority to work on the tokenizers package and see how we can go about using those tokenisers instead (for segmenting, not hashing), and also how we could write a "selection-based" (rather than segmentation-based) tokenizer around a spacyr tokenization.

kbenoit · 2017-07-28T07:06:28Z

    # didn't use accessor documents() because didn't want to pass
    # that large object
-    rownames(x$documents)
+    if (is.null(rownames(x$documents))) {


This seems slightly dangerous, since it means we could have a corpus object with no document names, but that docnames() will make it seem like it does have document names. Have you tested all of the functions that rely on there being actual document names? If they all call docnames.corpus() then we should be OK, since this will create the names.

Documents should have names always, but really old corpus object might not. This is a safeguard for errors in subsequent processes. If we were to give up backward compatibility, better to raise errors here.

kbenoit · 2017-07-28T07:08:56Z

-    thesDfm[1:10, (nfeature(thesDfm)-8) : nfeature(thesDfm)]
+    dictDfm <- dictDfm <- dfm(mycorpus, dictionary = mydict, valuetype = "glob")
+    dictDfm <- dictDfm[1:10, ]
+    dictDfm <- thesDfm <- dfm(mycorpus, thesaurus = mydict, valuetype = "glob")


I'm not sure what the point of these assignments are, since they keep overwriting dictDfm. Shouldn't there be some expect_*() calls around these to make them tests?

These are the oldest test for dfm that were not isolated. I do not see why they are still there either. I assigned output values to variables only because I do not want to set dfm to be printed to my console.

kbenoit · 2017-07-28T07:09:23Z

-    dfm(txt, thesaurus = mydict, verbose = FALSE)
-    dfm(txt, thesaurus = mydict, verbose = FALSE)
+    dictDfm <- dfm(txt, dictionary = mydict, verbose = FALSE)
+    dictDfm <- dfm(txt, thesaurus = mydict, verbose = FALSE)


Likewise, not sure what the point of these are, esp since one line is repeated.

I think we should delete all the code in test_that("oldest dfm test", {}).

I'll take a look at that.

kbenoit · 2017-07-28T07:09:39Z

-    dfm_lookup(txtDfm, mydict, valuetype = "glob") 
-    dfm_lookup(txtDfm, mydict, exclusive = FALSE, valuetype = "glob", verbose = FALSE) 
+    txtDfm <- dfm(txt, verbose = FALSE)
+    dictDfm <- dfm_lookup(txtDfm, mydict, valuetype = "glob") 


ditto re above

kbenoit · 2017-07-28T07:10:01Z

-    dfm(txt, stem = TRUE)
-    dfm(txt, remove = stopwords("english"))
-    dfm(txt, stem = TRUE, remove = stopwords("english"))
+    txtDfm <- dfm(txt)


here too - no tests here

kbenoit · 2017-07-28T07:29:24Z

 #' @export
 #' @noRd
-tokens.corpus <- function(x, ..., include_docvars = TRUE) {
+tokens.corpus <- function(x, ..., hash = TRUE, include_docvars = TRUE) {


Interesting - I thought we would get errors in the CHECK if not all the signatures are the same.

kbenoit · 2017-07-28T07:33:44Z

+        regex = c(regex, "^[\\p{S}]+$")
+    if (remove_separators)
+        regex = c(regex, "^[\uFE00-\uFE0F\\p{Z}\\p{C}]+$") 
+    if (remove_url)


with the exception of the remove_url, the others are char_class and might be faster implemented that way (through stringi::stri_remove_charclass() - although I realise that tokens_remove() does not currently provide this option. Something to note for the future? Or, we could examine the regex in tokens_remove() and if it matches a charclass, we call that instead of the equivalent regex function. We should test the performance differences before investing time in this.

I don't think there will be a much performance gain, but tokens_remove(toks, 'P', 'charclass') is certainly much easier than tokens_remove(toks, "^[\\p{P}]$", 'regex').

I was thinking we would stick to the stringi syntax, not create an interpretative layer in between. So it would be similar to

> stri_detect_charclass("Unicode symbol £", "[\\p{S}\\p{P}]") [1] TRUE > stri_detect_charclass("Unicode puncutation !", "[\\p{S}\\p{P}]") [1] TRUE

kbenoit · 2017-07-28T07:34:14Z

 #' @param concatenator character between multi-word expressions, default is the underscore character.  See Details.
 #' @return \code{as.tokens} returns a quanteda \link{tokens} object.
-#' @details The \code{concatenator} value is (TO BE COMPLETED).
+#' @details The \code{concatenator} is used to automatically generate dictionary values for multi-word expressions 


Please provide a bit more detail, and an (prose) example.

kbenoit · 2017-07-28T07:36:04Z

-                        "skip" = 0L, 
-                        "concatenator" = concatenator,
-                        "padding" = FALSE)
-    docvars(result) <- data.frame(row.names = docnames(result))


Wouldn't all this be easier if we had an S4 class for this with a prototype to set these slots?

kbenoit · 2017-07-28T07:37:27Z

    for (i in seq_along(x)) {

        if (verbose) catm("...tokenizing", i, "of" , length(x), "blocks\n")
-        if (what %in% c("word", "fastestword", "fasterword")) {


This look might be a natural thing to parallelise (in R).

koheiw · 2017-07-28T11:12:41Z

Since I separated special handling of hyphens and Twitter tags etc, tokens_word() became almost nothing. We can use tokenizers anytime.

kbenoit · 2017-07-28T12:01:02Z

Some dfm print tests are failing because the tokenisation behaviour has changed. Before we just rewrite the test expectations, should we investigate the differences so we are sure we understand them?

… dev-tokens

koheiw · 2017-07-28T12:48:46Z

I added old argument to tokens to compare with the version. The difference is from hyphenated words:

old <- unlist(tokens(data_char_ukimmig2010, old = TRUE), use.names = FALSE)
new <- unlist(tokens(data_char_ukimmig2010, old = FALSE), use.names = FALSE)
new2 <- unlist(tokens(data_char_ukimmig2010, remove_hyphen = TRUE, old = FALSE), use.names = FALSE)

setdiff(old, new)
setdiff(new, old)

setdiff(old, new2)
setdiff(new2, old)

> setdiff(old, new)
[1] "abiding"        "non"            "ill"            "educated"       "knee"           "jerk"           "300,000"       
[8] "represented"    "term"           "re"             "building"       "soft"           "touch"          "breakers"      
[15] "Abiding"        "sponsored"      "ethnicity"      "28"             "decision"       "creating"       "cross"         
[22] "boundary"       "E"              "longer"         "discriminatory" "quality"        "No"             "traffickers"   
[29] "co"             "ordinated"      "wide"           "point"          "scoring"        "recently"       "skill"         
> setdiff(new, old)
[1] "law-abiding"           "decades-long"          "non-British"           "ill-educated"          "knee-jerk"            
[6] "300,000-500,000"       "over-represented"      "Non-Immigration"       "non-immigration"       "long-term"            
[11] "re-building"           "second-generation"     "soft-touch"            "law-breakers"          "Law-Abiding"          
[16] "re-entry"              "government-sponsored"  "ethnicity-specific"    "28-style"              "second-class"         
[21] "decision-making"       "nation-building"       "civilisation-creating" "non-EU"                "cross-boundary"       
[26] "E-borders"             "longer-term"           "non-discriminatory"    "high-quality"          "Australian-style"     
[31] "points-based"          "police-level"          "public-sector"         "No-one"                "people-traffickers"   
[36] "high-priority"         "co-ordinated"          "EU-wide"               "point-scoring"         "recently-arrived"     
[41] "high-skill"            "five-year"             "time-limited"          "non-work"              "face-to-face"         
> setdiff(old, new2)
character(0)
> setdiff(new2, old)
character(0)

kbenoit · 2017-07-28T13:09:22Z

OK, nice. So it looks like most of the difference is coming from hyphens being preserved (correctly) in the new code, while being split (incorrectly) in the old code.

koheiw · 2017-07-28T13:32:34Z

It seems to be the case in Japanese texts too.

koheiw · 2017-07-28T13:44:57Z

This is just a confirmation:

microbenchmark::microbenchmark(
    old = tokens(corp, old = TRUE),
    new = tokens(corp, remove_hyphen = TRUE, old = FALSE),
    times = 10
)


Unit: seconds
 expr       min       lq     mean   median       uq      max neval
  old 14.301439 14.73900 15.08327 15.02283 15.65775 15.80192    10
  new  9.725151 10.22395 10.31369 10.41880 10.43882 10.86264    10

Merge branch 'dev-tokens' of https://github.com/kbenoit/quanteda into dev-tokens # Conflicts: # tests/testthat/test-dfm.R

… dev-tokens

koheiw added 13 commits July 27, 2017 15:20

rename file

d3a4cb4

update tests

30552b2

Add test for docnames

475e3ff

Rename

29cd262

docnames generate names when there aren't

7c43e5b

Update benchmark

a2a969d

Add test of tokens.tokens

97d38b7

Build

e215859

Fix test

c03e7fe

Make serialization more roboust

01c973c

Fix

188bb1d

Separate special handling of URL, Twitter, and hyphens

e10369e

Merge branch 'master' of https://github.com/kbenoit/quanteda into dev…

7b16772

…-tokens

koheiw added 2 commits July 27, 2017 22:54

Keep fastestword fast

074fbb2

Add examles for as.tokens()

6be28b3

kbenoit requested changes Jul 28, 2017

View reviewed changes

Add more explanaiton on concatenator

3102118

koheiw and others added 6 commits July 28, 2017 13:01

Merge branch 'master' into dev-tokens

a89c771

Tidy up message about concatenator

e4da0f8

Merge branch 'dev-tokens' of https://github.com/kbenoit/quanteda into…

f57f052

… dev-tokens

Change width of example in as.tokens

17b38b2

Add old function for comparison

53333e7

Merge branch 'dev-tokens' of https://github.com/kbenoit/quanteda into…

9e832c3

… dev-tokens

Changes for remove_hyphens

36a51a0

Fix

77a621b

kbenoit and others added 10 commits July 28, 2017 16:07

Fix tests in test-dfm.R for new tokens()

a561a18

Fix tests

d2641b5

Merge

ab061d5

Merge branch 'dev-tokens' of https://github.com/kbenoit/quanteda into dev-tokens # Conflicts: # tests/testthat/test-dfm.R

Fix tests in test-dfm_select

aad1a51

Merge branch 'dev-tokens' of https://github.com/kbenoit/quanteda into…

69f67d6

… dev-tokens

Fix test-textmodel_wordfish.R tests for new tokens()

35d1210

Fix test-textstat_dist.R test for new tokens()

7799f9e

Fix test-textstat_dist.R fo new tokens()

224ef1e

Update NEWS

71e0956

Update pkgdown site and index

9df1d6c

kbenoit merged commit c4dc679 into master Jul 28, 2017

kbenoit deleted the dev-tokens branch July 28, 2017 17:21

Uh oh!

Conversation

koheiw commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kbenoit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koheiw Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koheiw commented Jul 28, 2017

Uh oh!

kbenoit commented Jul 28, 2017

Uh oh!

koheiw commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbenoit commented Jul 28, 2017

Uh oh!

koheiw commented Jul 28, 2017

Uh oh!

koheiw commented Jul 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

koheiw commented Jul 27, 2017 •

edited

Loading

codecov Bot commented Jul 27, 2017 •

edited

Loading

koheiw Jul 28, 2017 •

edited

Loading

koheiw commented Jul 28, 2017 •

edited

Loading