Add an illustrative example to ?GForce when sorting locale matters #5342

MichaelChirico · 2022-02-28T05:35:27Z

codecov · 2022-02-28T06:53:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (64e2041) 97.48% compared to head (31f7fff) 97.48%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5342   +/-   ##
=======================================
  Coverage   97.48%   97.48%           
=======================================
  Files          80       80           
  Lines       14862    14862           
=======================================
  Hits        14488    14488           
  Misses        374      374

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tdhock · 2023-12-14T16:17:23Z

NEWS.md


 15. Thanks to @ssh352, Václav Tlapák, Cole Miller, András Svraka and Toby Dylan Hocking for reporting and bisecting a significant performance regression in dev. This was fixed before release thanks to a PR by Jan Gorecki, [#5463](https://github.com/Rdatatable/data.table/pull/5463). 

+17. Some clarity is added to `?GForce` for the case when subtle changes to `j` produce different results because of differences in locale. Because `data.table` _always_ uses the "C" locale, small changes to queries which activate/deactivate GForce might cause confusingly different results when sorting is involved, [#5331](https://github.com/Rdatatable/data.table/issues/5331). The inspirational example compared `DT[, .(max(a), max(b)), by=grp]` and `DT[, .(max(a), max(tolower(b))), by=grp]` -- in the latter case, GForce is deactivated owing to the _ad-hoc_ column, so the result for `max(a)` might differ for the two queries. An example is added to `?GForce`. As always, there are several options to guarantee consistency, for example (1) use namespace qualification to deactivate GForce: `DT[, .(base::max(a), base::max(b)), by=grp]` or (2) turn off GForce with `options(datatable.optimize = 0)`. Thanks @markseeto for the example and @michaelchirico for the improved documentation.


this is a great news item

Another option worth to mention is setting up locale to C

* Fix 5492 by limiting the costly deparse to `nlines=1` * Implementing PR feedbacks * Added inside * Fix typo in name * Idiomatic use of inside * Separating the deparse line limit to a different PR --------- Co-authored-by: Michael Chirico <[email protected]>

* Added my improvements to the intro vignette * Removed two lines I added extra as a mistake earlier * Requested changes

* fix typos and grammatical mistakes * fix typos and punctuation * remove double spaces where it wasn't necessary * fix typos and adhere to British English spelling * fix typos * fix typos * add missing closing bracket * fix typos * review fixes * Update vignettes/datatable-benchmarking.Rmd Co-authored-by: Michael Chirico <[email protected]> * Update vignettes/datatable-benchmarking.Rmd Co-authored-by: Michael Chirico <[email protected]> * Apply suggestions from code review benchmarking Co-authored-by: Michael Chirico <[email protected]> * remove unnecessary [ ] from datatable-keys-fast-subset.Rmd * Update vignettes/datatable-programming.Rmd Co-authored-by: Michael Chirico <[email protected]> * Update vignettes/datatable-reshape.Rmd Co-authored-by: Michael Chirico <[email protected]> * One last batch of fine-tuning --------- Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]>

* Updated documentation for rbindlist(fill=TRUE) * Print NULL entries of list as NULL * Added news item * edit NEWS, use '[NULL]' not 'NULL' * fix test * split NEWS item * add example --------- Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Benjamin Schwendinger <[email protected]>

* clarify that list input->unnamed list output * Add example where make.names is used * mention role of make.names

MichaelChirico · 2024-01-12T02:54:29Z

@TysonStanley are we in the thick of the release process already? Wondering if maybe this doc-only change should be shipped with 1.15.0.

It's not urgent though, so if you've already started packaging up 1.15.0 for CRAN, no need to bother -- we'll just merge to 1-15-99.

TysonStanley · 2024-01-12T05:27:14Z

Was waiting on a fix from @ben-schwen and @tdhock added the change in the description too so this would be just fine.

MichaelChirico · 2024-01-12T05:39:21Z

Was waiting on a fix from @ben-schwen

Is that different from #5887? That was merged / results in revdep show nothing new. Sorry but not sure I follow the second half of your comment.

TysonStanley · 2024-01-12T05:43:40Z

Great, then I would say to include this and then tomorrow I'll restart the release process.

As for the second part, Toby changed ctr to aut for you, Toby, and Jan as well but I had seen that was merged, just missed the one from Ben.

MichaelChirico · 2024-01-12T05:58:50Z

OK, rebased to master again. Ready for merge after final review.

MichaelChirico · 2024-01-12T06:00:59Z

As for the second part, Toby changed ctr to aut for you, Toby, and Jan as well but I had seen that was merged, just missed the one from Ben.

NB: Ben is not currently a committer: https://github.com/orgs/Rdatatable/teams/maintainers

TysonStanley · 2024-01-12T06:03:15Z

As for the second part, Toby changed ctr to aut for you, Toby, and Jan as well but I had seen that was merged, just missed the one from Ben.

NB: Ben is not currently a committer: https://github.com/orgs/Rdatatable/teams/maintainers

Right, I meant Ben's work on #5887 . Only you, Jan, and Toby are committers.

ben-schwen

GForce is already turned off when options(datatable.optimize=1L). Might want to adjust that or say that optimize=0 turns optimizations off

MichaelChirico added 3 commits February 27, 2022 21:33

improve documentation for GForce where sorting affects the result

49aa47e

link issue

e04d897

tests

6e1888c

Merge branch 'master' into gf-verbose

65ce11d

MichaelChirico requested a review from mattdowle as a code owner December 14, 2023 11:15

typo

1088aac

tdhock reviewed Dec 14, 2023

View reviewed changes

MichaelChirico and others added 6 commits December 14, 2023 18:46

mention Sys.setlocale

aa8f432

obsolete comment

49726b4

1.15.0 on CRAN. Bump to 1.15.99

dcacc58

Improvements to the introductory vignette (#5836)

f8a11af

* Added my improvements to the intro vignette * Removed two lines I added extra as a mistake earlier * Requested changes

MichaelChirico changed the base branch from master to 1-15-99 January 7, 2024 08:26

MichaelChirico added 2 commits January 7, 2024 16:27

Merge branch '1-15-99' into gf-verbose

eb71043

fix bad merge

59de28d

MichaelChirico requested a review from tdhock January 7, 2024 08:30

sritchie73 and others added 3 commits January 8, 2024 00:58

clarify that list input->unnamed list output (#5383)

251c660

* clarify that list input->unnamed list output * Add example where make.names is used * mention role of make.names

Merge branch '1-15-99' into gf-verbose

ee6080f

MichaelChirico requested a review from TysonStanley January 12, 2024 02:53

MichaelChirico changed the base branch from 1-15-99 to master January 12, 2024 05:39

Merge branch 'master' into gf-verbose

71a610f

MichaelChirico added 2 commits January 12, 2024 05:52

revert from next release branch

b511809

manual merge NEWS

a7a892a

MichaelChirico added 2 commits January 12, 2024 05:54

manual rebase tests

0d692be

manual rebase data.table.R

2eb195e

ben-schwen reviewed Jan 12, 2024

View reviewed changes

clarify 0 turns off everything

31f7fff

jangorecki approved these changes Jan 12, 2024

View reviewed changes

MichaelChirico merged commit cde7333 into master Jan 12, 2024

MichaelChirico deleted the gf-verbose branch January 12, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add an illustrative example to ?GForce when sorting locale matters #5342

Add an illustrative example to ?GForce when sorting locale matters #5342

Uh oh!

MichaelChirico commented Feb 28, 2022

Uh oh!

codecov bot commented Feb 28, 2022 •

edited

Loading

Uh oh!

tdhock Dec 14, 2023

Uh oh!

jangorecki Dec 14, 2023

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

ben-schwen left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants


		15. Thanks to @ssh352, Václav Tlapák, Cole Miller, András Svraka and Toby Dylan Hocking for reporting and bisecting a significant performance regression in dev. This was fixed before release thanks to a PR by Jan Gorecki, [#5463](https://github.com/Rdatatable/data.table/pull/5463).

		17. Some clarity is added to `?GForce` for the case when subtle changes to `j` produce different results because of differences in locale. Because `data.table` _always_ uses the "C" locale, small changes to queries which activate/deactivate GForce might cause confusingly different results when sorting is involved, [#5331](https://github.com/Rdatatable/data.table/issues/5331). The inspirational example compared `DT[, .(max(a), max(b)), by=grp]` and `DT[, .(max(a), max(tolower(b))), by=grp]` -- in the latter case, GForce is deactivated owing to the _ad-hoc_ column, so the result for `max(a)` might differ for the two queries. An example is added to `?GForce`. As always, there are several options to guarantee consistency, for example (1) use namespace qualification to deactivate GForce: `DT[, .(base::max(a), base::max(b)), by=grp]` or (2) turn off GForce with `options(datatable.optimize = 0)`. Thanks @markseeto for the example and @michaelchirico for the improved documentation.

Add an illustrative example to ?GForce when sorting locale matters #5342

Add an illustrative example to ?GForce when sorting locale matters #5342

Uh oh!

Conversation

MichaelChirico commented Feb 28, 2022

Uh oh!

codecov bot commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tdhock Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

jangorecki Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

MichaelChirico commented Jan 12, 2024

Uh oh!

TysonStanley commented Jan 12, 2024

Uh oh!

ben-schwen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Feb 28, 2022 •

edited

Loading