Issue 1840 2

[koheiw]

Based on #2045, I made the output of kwic() identical to the v2. It calls locate() to get positions of keywords. Users are recommended to use locate() before running kwic() to get ideas how many times the patterns hit when they analyze large corpora.

It is named locate() simply because there are find() and match() in base R. I am happy with more creative names.

[kbenoit]

So locate() is essentially the kwic() from the other Issue 1840 PR, that includes the tokens, but kwic() drops them and returns the same sort of data.frame we had formerly? Will having the locate function separate provide user-facing functionality? or is this intended to be internal only?

I'm fine with kwic() being unchanged from the user standpoint, although it's better now overall from the work we did on the previous PR.

Obviously there a lot of tests that would need redoing, and documentation for locate() if you meant to export this. (You can't noRd exported functions.)

BTW I think this would be simpler to review and fix if we merged the other PR, then made this a new PR, but happy to work with it either way.

[koheiw]

locate() should be exported as the fastest function to find patters. I also think that the developers like the tidytext-like long format.

> microbenchmark::microbenchmark(
+     locate(toks, data_dictionary_LSD2015),
+     tokens_select(toks, data_dictionary_LSD2015),
+     tokens_lookup(toks, data_dictionary_LSD2015),
+     kwic(toks, data_dictionary_LSD2015),
+     times = 1, unit = "relative"
+ )
Unit: relative
                                         expr       min        lq      mean    median        uq       max neval
        locate(toks, data_dictionary_LSD2015)  1.000000  1.000000  1.000000  1.000000  1.000000  1.000000     1
 tokens_select(toks, data_dictionary_LSD2015)  1.586143  1.586143  1.586143  1.586143  1.586143  1.586143     1
 tokens_lookup(toks, data_dictionary_LSD2015)  1.813328  1.813328  1.813328  1.813328  1.813328  1.813328     1
          kwic(toks, data_dictionary_LSD2015) 26.129500 26.129500 26.129500 26.129500 26.129500 26.129500     1

[kbenoit]

OK, this looks really good, and the performance is great. This could be a very useful function for building upon, the same way that you've redefined kwic to work on this. We can see if it's possible to build on the kwic.locate() performance.

I'm happy to help clean this up but how about first, I merge the other PR, then merge master into this branch. that will leave just your two commits as the diff, and we can use those to fix the documentation, tests, etc.?

[koheiw]

Glad you liked it. It would be great if you could update the tests and man, but should not be difficult, because it is only to change kwic() to locate(), and as.data.frame.kiwc() to kwic().

Also, we might want to change the name to location() (and the class label) as almost all the current exported functions are nouns.

[kbenoit]

Happy to clean this up, since that gives me a good, hands-on opportunity to get used to the new functions and get additional insights into their potential.

True on the nouns, although that's more for our core object constructors. This is a utility a bit more like convert(), so happy to keep as locate(). Maybe locate_tokens()?

Should this be keyworded internal, or should it appear in the online documentation index?

[kbenoit]

Ok I tidied this up, after merging the other PR so that we could focus on the changes introduced through the use of the new locate() function.

I consolidated the formerly kwic.locate() into kwic.tokens() since that's the only function that will wrap the kwic creation around a locate return object. If we did have a separate function, it would be as.kwic.locate() - but I don't think we need a separate function. Your call.

I did not think we still need to include the tokens object in the kwic return, since no functions would use this. A simple vector of ntoken counts is enough for textplot_xray(), the only complex function that takes a kwic input that we currently have. So I redefined the structure to include just the attribute ntoken, and only for the documents for which matches exist. (I verified that this works with the current CRAN version of quanteda.textplots.)

Because there is an attribute for the kwic return object, I revived as.data.frame.kwic() since that strips the attributes.

Questions/issues:

• Do we need to include the tokens object in the locate return? Depends on what you think we will use this output for.
• Are we sure on the name? Alternatives:
  • location()
  • find()
  • find_tokens()
    I like find() or find_tokens().

[codecov]

Codecov Report

Merging #2065 (28beb9e) into master (40cb35e) will decrease coverage by 0.01%.
The diff coverage is 98.18%.

@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
- Coverage   95.67%   95.65%   -0.02%     
==========================================
  Files          84       85       +1     
  Lines        4903     4905       +2     
==========================================
+ Hits         4691     4692       +1     
- Misses        212      213       +1     

Impacted Files	Coverage Δ
R/index.R	95.00% <95.00%> (ø)
R/kwic.R	98.71% <100.00%> (-0.23%)	⬇️
src/index_mt.cpp	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40cb35e...2b99652. Read the comment docs.

[koheiw]

It is cleaner without tokens attached to the location object, but this would means that we will add a new input to patterns: kwic(toks, pattern = location). This is not a bad idea if the location object will work with other functions like tokens_select(toks, pattern = location).

If we were to add the location object, location_match(), location_find() or pattern_find() would be better.

[kbenoit]

I think we should not redefine pattern in that way because pattern is expected to operate with the pattern, valuetupe, case_insensitive trio. If pattern included the location table, then it breaks that setup. This is one of the reasons we removed dfm from being an input to pattern, for instance.

We could add an option for index or location to some tokens lookup functions, as an alternative to pattern.

But all of these options involve adding things that can be done within the framework set up by this PR. So I suggest we don't need to resolve them here. This PR further improves kwic() and adds additional functionality via locate(). We can think about how to extend the locate() functionality separately.

So I'm in favour of merging this, but renaming the function index(), or find_tokens(). We mark it as experimental, meaning it could change. For instance it could be the basis of an index for all tokens, or an index of named entities, or the locations of annotation spans. There is a lot we could do with it, but we will need time to think about it and explore options.

[koheiw]

OK. Let's name it index() that returns an index object. It will be passed to the index argument in kwic() and others.

[koheiw]

It is done, but no idea why it is failing the test on Windows.

[kbenoit]

Something strange happening with the windows build of quanteda.textplots (and windows builds generally - not sure what it is but seems to be a CRAN issue). I think we can ignore it.

[koheiw]

Please merge if you think it is good enough.

[kbenoit]

OK. One thought before we finalise this: should we convert an index object to kwic using kwic() with new the index argument, or should this be as.kwic.index()? The as.kwic() would make it unnecessary to change kwic() and might be a more natural way to deliver what is essentially just an addition to the index of the context works.

[koheiw]

It depends on how much we want to promote the index object. as.kwic.index(x, y) (where y is a tokens object) does the job, but I have the feeling that the index object has many potential use-cases with other tokens_* functions.

[kbenoit]

The index object has enormous potential, but we haven't yet figured out all of the best ways to use it or integrate it into other functionality. My conservative suggestion is to keep the functionality separate for now in a coercion function (as.kwic()) since this gives us the maximum flexibility later. Meaning: we don't need to modify kwic()'s signature now, or possibly face undoing it later if we decide some other way to make use of index objects in other functions.

[koheiw]

That is fine with me for now. Please make changes as you suggested.

[kbenoit]

Well, it became immediately clear once I started implementing as.kwic.index() that this cannot work without attaching the original tokens object as an attribute to the index, or passing the tokens object as an argument to as.kwic.index().

It does make the kwic.tokens() code compact, since we just do the checking, then call index(x, ...) then as.kwic.index() on the result. But for this to work, we would need:

as.kwic.index <- function(x, tokens, window = 5L)

But then we are not coercing an object, we are constructing it from two inputs plus a parameter. So I think I prefer your solution in the last few commits here, where index is an optional argument. We mark this as experimental in case we further develop indexing, which I think is safe since few if any users are likely to use it for a while at least.

[koheiw]

I said that y should be tokens in as.kwic()... I am happy with the kwic() with index.