Update dfm

[koheiw]

Call dfm.tokens_xptr() from dfm.tokens() for #2249. There are many changes and tests accordingly.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.47 🎉

Comparison is base (2b4d07a) 95.66% compared to head (620360e) 96.13%.

❗ Current head 620360e differs from pull request most recent head a8c2ea2. Consider uploading reports for the commit a8c2ea2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2251      +/-   ##
==========================================
+ Coverage   95.66%   96.13%   +0.47%     
==========================================
  Files          97       97              
  Lines        5600     5435     -165     
==========================================
- Hits         5357     5225     -132     
+ Misses        243      210      -33     

Impacted Files	Coverage Δ
R/dfm-methods.R	97.05% <ø> (ø)
R/dfm_lookup.R	95.16% <ø> (ø)
R/dfm_match.R	100.00% <ø> (ø)
R/dfm_replace.R	91.66% <ø> (ø)
R/dfm_sort.R	78.57% <ø> (ø)
R/dfm_trim.R	93.18% <ø> (ø)
R/dfm_weight.R	95.76% <ø> (ø)
R/dictionaries.R	94.24% <ø> (ø)
R/docnames.R	100.00% <ø> (ø)
R/bootstrap_dfm.R	100.00% <100.00%> (ø)
... and 15 more

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[koheiw]

@kbenoit can you review this?

[kbenoit]

This is a nice improvement and makes the underlying engine all based on xptr. It also removes the option of not doing that. Since the dfm is the same object, however, this should not affect compatibility, since a v3 dfm and a v4 dfm will be the same objects. (But do correct me if I'm wrong here!)

Two comments.

1. As I always emphasise, deprecations should be separate from functionality changes, whenever they are not integral to functionality change. Pruning is satisfying (I know!) but creates untold headaches for me in particular. The most time-consuming part of releasing v3 was me notifying other package maintainers of breaking changes and often, issuing my own PRs to fix code in their packages that would be broken by even our warnings. There is also the issue of the user base and their code. I agree we will defunct anything deprecated in v3, but there are some nifty tools for doing this now, and they are far more informative than just .Defunct(). (See https://r-pkgs.org/lifecycle.html) So I prefer to do these in a separate series of PRs because this is the most delicate surgery. That means I'd prefer the removals not be removed here. There is a lot that is good that you tidied here, including some tests we suppressed the warnings on that I agree should be removed here, but not simply removing code for v3 deprecations.

2. I still don't like the nomenclature of tokens_xptr. It is not consistent with our single-word object names, and the use of the underscore. It's not a function we have to call, I know, so I'm not 100% convinced it's a problem, but it's inconsistent - and consistency is one of the beautiful things about quanteda.

So I don't like seeing this for instance:

Error: dfm() only works on dfm, tokens, tokens_xptr objects.

Can we name this child class of tokens something that is a single word? tokensx or even just remove the underscore so it's tokensxptr? (I prefer the first.)

But it's not a deal-breaker.

[koheiw]

Thanks for reviewing. I decided to remove deprecated functions to to reduce the workload. It does not make sense to write code for functions that will be removed. Introduction of the tokens_xptr object as you can see in the number of commits that I made in the last three months.

[koheiw]

There should not be any function that tokenize texts on the fly like bootstrap_dfm() and nsentecens() etc. This is why I removed. Uses should pass a DFM to bootstrap_dfm().

[koheiw]

More generally, we should reduced the number of functions more aggressively. It is becoming too hard to maintain. I am the only person who is doing the substantive development. Otherwise, I will really create quanteda.core package and leave functions that I think redundant with you to maintain.

[koheiw]

Please also note that despite the removed tests, the test coverage is higher significantly. We should keep removing redundant tests and examples too. I don't delete important tests.

[kbenoit]

@koheiw I'm so deeply impressed by how much time that you are putting into the package development - and the spectacular performance gains they have brought - that I almost messaged you out of concern whether you still have a job! (I hope so...)

If there are functions you think should be removed, propose them in an issue. But please don't remove them from a performance-related PR because this slows down the assessment of the PR. We can discuss and remove them separately. Anything that generates errors or warnings in rev deps is one of the hardest things to test for CRAN compliance. And most time-consuming to fix. I don't mind doing it but I prefer to minimise the post-surgery recovery time!

[koheiw]

I choose the name toknes_xptr for the following reasons when I started developing.

1. The new tokens object inherits properties of tokens and externalptr objects, so the class label could be c("externalptr", "tokens"), but it triggers methods for externalptr when I don't want to. If c("tokens", "externalptr"), I have to check the second class label in all the S3 methods for tokens.
2. I need a special class label for the new label that tell the users the nature of the object like "tokens_externalptr". It could be "tokensx", "xtokens" to make it short, but then I have to make a functions called tokensx_*() or xtokens_*(). It could be tokens-xptr or tokens.xptr but we know that the underscore is the best for S3 class labels.
3. It cannot be "tokens2" because "tokens" will co-exist with the new object.
4. We already have class labels with underscores like "textstat_proxy", "textstat_simil_sparse".

[koheiw]

By the way, this is not a PR for performance improvement. I fixed the C++ function on this branch because replacing R code with C++ code broke many tests.

[koheiw]

I decided to develop v4.0 because the data that I analyze on my job is getting larger and larger.

[kbenoit]

OK, all good points, let's do this then.

1. We stick with the class name, since what you say makes sense, and I was already thinking about the underscored classes in textstats and textmodels as precedents, so let's stick with tokens_xptr.
2. I'll restore the removed items that are not integral to the PR, but remove them in a separate PR so that I can test their effects, and not hold up things further. Then we can merge this PR and move on to the rest of v4 plan.

[kbenoit]

I'm going to start running revdep_check() with each PR but let's merge it first and I'll start the process then.