Setlevels #4197

2005m · 2020-01-24T15:43:20Z

codecov · 2020-01-24T15:58:41Z

Codecov Report

Merging #4197 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4197      +/-   ##
==========================================
+ Coverage   99.61%   99.61%   +<.01%     
==========================================
  Files          72       72              
  Lines       13876    13952      +76     
==========================================
+ Hits        13822    13898      +76     
  Misses         54       54

Impacted Files	Coverage Δ
R/wrappers.R	`100% <ø> (ø)`	⬆️
R/merge.R	`100% <ø> (ø)`	⬆️
src/subset.c	`100% <ø> (ø)`	⬆️
R/as.data.table.R	`100% <ø> (ø)`	⬆️
src/assign.c	`100% <ø> (ø)`	⬆️
src/init.c	`100% <ø> (ø)`	⬆️
src/fwriteR.c	`100% <100%> (ø)`	⬆️
src/dogroups.c	`100% <100%> (ø)`	⬆️
src/frank.c	`100% <100%> (ø)`	⬆️
R/frank.R	`100% <100%> (ø)`	⬆️
... and 19 more

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 0c661b4...55e6111. Read the comment docs.

R/wrappers.R

MichaelChirico · 2020-01-24T16:48:15Z

man/setattr.Rd

@@ -24,6 +26,8 @@ setnames(x,old,new,skip_absent=FALSE)

  \code{setattr} is a more general function that allows setting of any attribute to an object \emph{by reference}.

+  \code{setlevels} is a function to set the levels of factor vector.


I feel this isn't needed as Details. If anything in Details, we should note the sort of consistency checks that are being run that make it advantageous vs setattr

MichaelChirico · 2020-01-24T16:49:07Z

inst/tests/tests.Rraw

@@ -16770,6 +16770,8 @@ test(2132.2, fifelse(TRUE, 1, s2),       error = "S4 class objects (except nanot
 test(2132.3, fcase(TRUE, s1, FALSE, s2), error = "S4 class objects (except nanotime) are not supported. Please see https://github.com/Rdatatable/data.table/issues/4131.")
 rm(s1, s2, class2132)

+# setlevels, #2219
+test(2133.1, setlevels(as.factor(c("A", "A", "B", "B", "B", "C")), c("X", "Y", "Z")), as.factor(c("X", "X", "Y", "Y", "Y", "Z")))


The issue was written specifically about how setlevels improves vs setattr because of the consistency checks; can we add some tests of these here?

Now that I look in more details at setattrib and setlevels I am a bit confused by these functions. Maybe you can shed some light on the following code :

x = as.factor(c("A","A","B","B","B","C")) print(setlevels(x, c("X","X","Y","Z"))) # X X X X X Y # Levels: X Y Z

That is not correct, is it? setattr behaves the same as it is built on top of setlevels.

Do I need to implement some checks at C level? I am happy to do it but I am bit confused now. I think I went a bit too fast!

Indeed that looks wrong!

I think my understanding of the motivation for setlevels was to error for that example, since X is duplicated. If not an error, unique(value) should be used.

Well that is not a problem, I can make it error :) . Anything else?

Just to be clear, that is what I have with setattr as well:

print(setattr(x, "levels", c("X","X","Y","Z"))) # [1] X X X X X Y # Levels: X Y Z

I can correct everything if you tell me exactly what each function is suppose to do. Thanks

Made some changes, let me know if anything else is needed. Thanks.

R/wrappers.R

src/wrappers.c

mattdowle · 2020-02-18T07:12:59Z

R/data.table.R

@@ -2457,6 +2457,14 @@ setattr = function(x,name,value) {
  invisible(x)
 }

+setlevels = function(x, value) {
+  if (anyDuplicated(value) == 0) {


I think we have an internal any_duplicated (or completely different name) which is specialized for character strings, and faster than base. If so, this anyDuplicated call should be moved down to C to use the internal one.

Excited to see setlevels() finally be exported :-)

I completely re-wrote setlevels please let me know what you think.

Removing the WIP except if you think it needs further changes?

mattdowle · 2020-02-18T07:21:49Z

How about making the setlevels interface just like setnames; e.g. with old and new? That way, the common task of renaming one or two levels could be facilitated too. setlevels could reuse the existing code in setnames, either by calling setnames, or maybe the internals of setnames would need to be extracted to a helper function which both setlevels and setnames would call.

2005m · 2020-02-18T12:03:25Z

I like your proposal but I am worried that we will break the setattr function which rely on Csetlevels. I think I am going to write a seperate function for you and rename the current one. That will be cleaner.

jangorecki

please add tests for cases when

there are factor levels for which data are not present factor("a", levels=c("a","b"))
factor levels are not ordered factor("a", levels=c("b","a"))
there are NAs in input factor(c("a",NA), levels=c("b","a"))
there are NAs in input and in factor levels factor(c("a",NA), levels=c("b","a",NA), exclude=NULL)
no NAs in input but in factor levels factor(c("a"), levels=c("b","a",NA), exclude=NULL)
as well for setting those NAs levels

inst/tests/tests.Rraw

man/setattr.Rd

jangorecki · 2020-02-29T02:56:50Z

src/wrappers.c

+    for (int64_t i=0; i<nx; ++i) {
+      if (STRING_ELT(xchar, i) == STRING_ELT(old_lvl, j)) {
+        SET_STRING_ELT(xchar, i, STRING_ELT(new_lvl, j));
+        goto label;


couldn't we avoid goto in favor of breaking the loop?

We could yes. I just fund it cleaner to use goto. break use goto under the hood I think. If you are not confortable with goto i can change it. No issue. I use goto to jump the error message mainly. I would need to create a variable to avoid it otherwise.

FWIW in our source we're only using goto in fread.c so far. not sure if Matt has a stylistic judgment on it

jangorecki · 2020-02-29T02:57:58Z

src/wrappers.c

+    error("Element '%s' of 'old' does not exist in 'x'.", CHAR(STRING_ELT(old_lvl, j)));
+    label:;
+  }
+  SEXP ans = PROTECT(duplicate(x)); nprotect++;


@mattdowle do we need copyAsPlain here? x is INTSXP so eventually could be a compact 1:n altrep sequence

2005m · 2020-02-29T06:10:06Z

Thanks @jangorecki for the feedback. Adding back the WIP flag as I won't be able to look at the above in the coming days.

jangorecki

It would be much easier to read those tests if they would have a comment at the end line what they mean to test. Just mentioning for future, no need to add now.
If those new tests has been added based on the expected output, and not the current output of setlevels, then it should be good.

R/wrappers.R

2005m added 6 commits January 24, 2020 14:54

wrapper

2b0cddb

NAMESPACE

58b21cf

Rd

aed1d30

tests

c390129

Oops

d0ce74c

News

7218828

2005m added this to the 1.12.9 milestone Jan 24, 2020

2005m requested review from jangorecki and mattdowle January 24, 2020 16:00

MichaelChirico reviewed Jan 24, 2020

View reviewed changes

R/wrappers.R Outdated Show resolved Hide resolved

grammar

b33aa0a

MichaelChirico reviewed Jan 24, 2020

View reviewed changes

return x

5822b00

2005m added the WIP label Jan 24, 2020

jangorecki reviewed Jan 25, 2020

View reviewed changes

R/wrappers.R Outdated Show resolved Hide resolved

changes

0c661b4

2005m removed the WIP label Jan 26, 2020

Merge branch 'master' into setlevels

cfe8049

mattdowle reviewed Feb 18, 2020

View reviewed changes

src/wrappers.c Outdated Show resolved Hide resolved

mattdowle reviewed Feb 18, 2020

View reviewed changes

mattdowle added the WIP label Feb 18, 2020

revamp

64ea9a4

2005m removed the WIP label Feb 28, 2020

jangorecki reviewed Feb 29, 2020

View reviewed changes

2005m added the WIP label Feb 29, 2020

test+skip_absent

92f1a1b

jangorecki reviewed Mar 3, 2020

View reviewed changes

R/wrappers.R Outdated Show resolved Hide resolved

2005m added 2 commits March 3, 2020 10:13

Remove line

0223c04

News

55e6111

2005m removed the WIP label Mar 3, 2020

2005m closed this Apr 8, 2020

2005m deleted the setlevels branch April 8, 2020 13:20

jangorecki modified the milestones: 1.12.11, 1.12.9 May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setlevels #4197

Setlevels #4197

2005m commented Jan 24, 2020

codecov bot commented Jan 24, 2020 •

edited

Loading

MichaelChirico Jan 24, 2020

MichaelChirico Jan 24, 2020

2005m Jan 24, 2020

MichaelChirico Jan 24, 2020

2005m Jan 24, 2020 •

edited

Loading

2005m Jan 24, 2020

2005m Jan 25, 2020

mattdowle Feb 18, 2020 •

edited

Loading

2005m Feb 18, 2020

2005m Feb 28, 2020

mattdowle commented Feb 18, 2020 •

edited

Loading

2005m commented Feb 18, 2020

jangorecki left a comment

jangorecki Feb 29, 2020

2005m Mar 2, 2020 •

edited

Loading

MichaelChirico Mar 3, 2020

jangorecki Mar 3, 2020

jangorecki Feb 29, 2020

2005m commented Feb 29, 2020

jangorecki left a comment •

edited

Loading

		@@ -24,6 +26,8 @@ setnames(x,old,new,skip_absent=FALSE)

		\code{setattr} is a more general function that allows setting of any attribute to an object \emph{by reference}.

		\code{setlevels} is a function to set the levels of factor vector.

Setlevels #4197

Setlevels #4197

Conversation

2005m commented Jan 24, 2020

codecov bot commented Jan 24, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2005m Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdowle Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdowle commented Feb 18, 2020 • edited Loading

2005m commented Feb 18, 2020

jangorecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2005m Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2005m commented Feb 29, 2020

jangorecki left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2020 •

edited

Loading

2005m Jan 24, 2020 •

edited

Loading

mattdowle Feb 18, 2020 •

edited

Loading

mattdowle commented Feb 18, 2020 •

edited

Loading

2005m Mar 2, 2020 •

edited

Loading

jangorecki left a comment •

edited

Loading