Closes #3639 -- proper handling of CPLXSXP in dogroups #3689

MichaelChirico · 2019-07-10T06:32:50Z

Haven't quite figured out the ins and outs of CPLXSXP and COMPLEX() usage yet but I think this gets at the kernel of the issue -- dogroups assumes SIZEOF is either 4 or 8, not so for CPLXSXP (16). Will come back to this.

FWIW grep -Er "==\s?4" src only turns up reorder and dogroups, so I think this and #3688 should cover this class of issue.

remove debugging helpers remove debugging helpers remove debugging helpers

codecov · 2019-07-10T11:14:04Z

Codecov Report

Merging #3689 into master will decrease coverage by 0.02%.
The diff coverage is 85%.

@@            Coverage Diff             @@
##           master    #3689      +/-   ##
==========================================
- Coverage   98.25%   98.23%   -0.03%     
==========================================
  Files          69       69              
  Lines       13088    13099      +11     
==========================================
+ Hits        12860    12868       +8     
- Misses        228      231       +3

Impacted Files	Coverage Δ
R/data.table.R	`97.87% <ø> (ø)`	⬆️
src/dogroups.c	`92.42% <85%> (-0.68%)`	⬇️

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 4e3868b...0288c5f. Read the comment docs.

codecov · 2019-07-10T11:14:05Z

Codecov Report

Merging #3689 into master will decrease coverage by 0.01%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #3689      +/-   ##
==========================================
- Coverage   98.25%   98.24%   -0.02%     
==========================================
  Files          69       69              
  Lines       13094    13122      +28     
==========================================
+ Hits        12866    12892      +26     
- Misses        228      230       +2

Impacted Files	Coverage Δ
R/data.table.R	`97.87% <ø> (ø)`	⬆️
src/subset.c	`100% <ø> (ø)`	⬆️
src/init.c	`100% <100%> (ø)`	⬆️
src/assign.c	`100% <100%> (ø)`	⬆️
src/dogroups.c	`92.85% <73.68%> (-0.25%)`	⬇️

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 d3e146d...5258421. Read the comment docs.

src/dogroups.c

MichaelChirico · 2019-07-10T11:42:31Z

The code is working, just need to improve coverage (any ideas?)

src/dogroups.c

MichaelChirico · 2019-07-11T16:37:11Z

src/dogroups.c

        double *td = REAL(target);
        double *sd = REAL(source);
        for (int r=0; r<maxn; ++r) td[ansloc+r] = sd[igrp];
+      } else {


this one is in the ngrpcols loop, so covering it requires being able to group by complex, which we can't (yet)

MichaelChirico · 2019-07-11T23:42:59Z

src/dogroups.c

+        case CPLXSXP :
+          COMPLEX(VECTOR_ELT(xSD, j))[0].r = NA_REAL;
+          COMPLEX(VECTOR_ELT(xSD, j))[0].i = NA_REAL;
+          break;


IIUC this is coming from a join, so we won't be able to cover it yet.

Minimal example to trigger the INTSXP branch of this switch (adapted test 1283.3):

dt1 <- data.table(a=rep(1:2, each=2), c=c(7L, 2L, 4L, 8L)) dt2 <- data.table(b=rep(2:3), c=c(2L, 6L), d=c(20L, 8L)) dt1[dt2, on = c(a = 'b'), .(a=a), by=.EACHI]

I believe it's because j uses a column from the LHS of on.

But joins aren't available for complex yet...

MichaelChirico · 2019-07-11T23:44:22Z

OK. The remaining codecov problems I think are outside the scope of this PR (see comments). so I guess we should just wait to merge this one until after #3701, at which point we can add tests here.

Will also file a coverage commit for dogroups overall while I'm in here.

MichaelChirico · 2019-07-12T00:00:47Z

inst/tests/tests.Rraw

+DT3 = DT3[sample(nrow(DT3))]
+DT1[ , x_num := match(x, letters) + .1]
+DT3[ , x_num := match(x, letters) + .1]
+test(1540.36, DT1[DT3, .(x_num), by=.EACHI, on=c(x_num="x_num")])


strangely enough, I couldn't get this test to hit the LGLSXP branch:

DT1[ , x_lgl := x > = 'a'] DT3[ , x_lgl := x > 'a'] DT1[DT3, .(x_lgl), by=.EACHI, on=c(x_lgl="x_lgl")]

doesn't do it...

MichaelChirico · 2019-07-12T00:02:07Z

src/dogroups.c

+        case CPLXSXP :
+          COMPLEX(VECTOR_ELT(xSD, j))[0].r = NA_REAL;
+          COMPLEX(VECTOR_ELT(xSD, j))[0].i = NA_REAL;
+          break;
        case STRSXP :
          SET_STRING_ELT(VECTOR_ELT(xSD,j),0,NA_STRING);
          break;
        case VECSXP :
          SET_VECTOR_ELT(VECTOR_ELT(xSD,j),0,R_NilValue);
          break;


Should we nocov this branch (or set it to error even?)

IIUC hitting this branch would require first joining on VECSXP column in on, I don't think we have any plan to support list-to-list joins?

don't no cov it otherwise it'll drop off our todo list. I'd say don't error either because something somewhere might be using it. I'd say just leave it as-is for now until it can be covered at a later date.

…o cplx_dogroups

MichaelChirico · 2019-07-13T20:05:04Z

src/assign.c

@@ -1002,6 +1011,11 @@ void writeNA(SEXP v, const int from, const int n)
      for (int i=from; i<=to; ++i) vd[i] = NA_REAL;
    }
  } break;
+  case CPLXSXP: {
+    Rcomplex *vd = COMPLEX(v);
+    Rcomplex NA_CPLX = {NA_REAL, NA_REAL};


a bit strange why we have to define this ourselves but I see it done like this in a few places in r-source (e.g. subset.c)

Yes I see 3 in src/main/subset.c too. Those are the only occurence of the string "NA_C" (to catch both NA_COMPLEX, NA_CPLX, and anything similar starting with C for complex) in the entire source tree of R. So since it's so rarely used, I guess it just hasn't been moved into a header. Which seems reasonable. More to the point, because it needs to be a variable, it can't be plain compiled constant like the other NA_* definitions.

mattdowle · 2019-07-16T20:12:23Z

Have we heard from many users requesting complex support?
I'll try and get through these PRs asap, and hopefully they are quick, but it's adding complexity (pun intended) when another approach could be just to not support complex yet until other higher value things are done first.
If the idea is to pick off something where you can learn the code base, that's a good idea, and I'm ok to merge them in on that basis. Just hoping these 6 PRs are quick and they don't cause unintended effects.

Some links to users and fields requesting complex support please, would keep me happy that it's worth doing.

eliocamp · 2019-07-16T21:33:47Z

I'm one happy data.table user that uses complex numbers frequently and would be even happier if complex support is enhanced 😄. Perusing github I see there are some issues addressing it:

Not a lot (understandably), but non-zero.

mattdowle · 2019-07-16T21:54:14Z

@eliocamp Great, thanks. I looked through them. It looks to me as if it's programmers checking that all types are supported for completeness reasons, rather than users in the real-world actually needing complex number support. I did find one field mentioned in those issues though :

signal processing, especially for the analysis of power spectra.

What's your field that you use complex numbers in?

eliocamp · 2019-07-16T22:05:57Z

Meteorology, but I use some signal processing methods to analyse fields. Complex Principal Component Analysis, for example.

mattdowle · 2019-07-16T22:18:59Z

Good to know! Are there any articles that show how meteorology, signal processing and/or complex-PCA need complex numbers in the context of data.table-like operations like grouping? I did complex number theory in undergraduate applied math and much enjoyed it, but I've never used complex numbers since then.

mattdowle · 2019-07-16T22:36:51Z

All looks good. I realize the diff coverage is under target but that's ok, because as Michael wrote, other PRs will cover those in future.

MichaelChirico · 2019-07-16T23:56:14Z

pick off something where you can learn the code base

in fact that's the main thinking. I imagine there are some use cases around Fourier series running around but yea, I haven't come across it myself in a while either

MichaelChirico · 2019-07-17T00:00:23Z

src/subset.c

@@ -77,7 +77,6 @@ static void subsetVectorRaw(SEXP ans, SEXP source, SEXP idx, const bool anyNA)
  case CPLXSXP : {
    Rcomplex *sp = COMPLEX(source);
    Rcomplex *ap = COMPLEX(ans);
-    Rcomplex NA_CPLX = { NA_REAL, NA_REAL };


looking at it again, would just inlining = (Rcomplex){NA_REAL, NA_REAL} do the trick?

I don't think (Rcomplex){NA_REAL, NA_REAL} will work. Because Rcomplex is a struct. The {NA_REAL, NA_REAL} is special syntax just for initialization of a variable/struct.

eliocamp · 2019-07-17T01:13:26Z

Good to know! Are there any articles that show how meteorology, signal processing and/or complex-PCA need complex numbers in the context of data.table-like operations like grouping? I did complex number theory in undergraduate applied math and much enjoyed it, but I've never used complex numbers since then.

I've never read anything specific about data.table. Here's a paper about the method: https://journals.ametsoc.org/doi/abs/10.1175/1520-0450%281984%29023%3C1660%3ACPCATA%3E2.0.CO%3B2

This is the code I'm using now. It gets the Hilbert transform of the data and then does an EOF (Empirical Orthogonal Functions --what we meteorologists call PCA) for each season, dataset and height level. Because of #3639 I had to use the workaround of separating the real an imaginary parts and then joining them together after the grouping was done.

ceof <-  data %>%
  .[, c("R", "I") := ReIm(spectral::analyticFunction(hgt.minus)),
    by = .(lat, season, year, dataset, lev)] %>% 
  .[, {
    d <- copy(.SD)[, hgt := R + I*1i]
    list(ceof = list(
      EOF(hgt ~ year | lon + lat, n = 1:2, 
          B = 5000, suffix = "cPC", data = d)
    ))
  }, by = .(dataset, season, lev)]

EOF() (a function I created and available in the metR package) returns a list of data.tables that are the matrices of the Singular Value Decomposition in "long" format. So ceof is a data.table with a column names "ceof" (yeah, not very smart naming convention) that that holds said list of data.tables, one for each combination of dataset, season and height level.

mattdowle · 2019-07-17T23:32:02Z

Thanks @eliocamp, helps a lot! Continuing here: #3701 (comment)

MichaelChirico force-pushed the cplx_dogroups branch 2 times, most recently from fd4b465 to c17bb81 Compare July 10, 2019 06:40

initial take at #3639 -- proper handling of CPLXSXP in dogroups

bf4fb10

remove debugging helpers remove debugging helpers remove debugging helpers

MichaelChirico force-pushed the cplx_dogroups branch from c17bb81 to bf4fb10 Compare July 10, 2019 06:41

Michael Chirico added 2 commits July 10, 2019 18:24

main issue was name collision of I with that in complex.h; now working

24c8849

typo

0288c5f

MichaelChirico commented Jul 10, 2019

View reviewed changes

src/dogroups.c Show resolved Hide resolved

add CPLXSXP case to some switches

3f99784

MichaelChirico commented Jul 10, 2019

View reviewed changes

src/dogroups.c Outdated Show resolved Hide resolved

MichaelChirico commented Jul 10, 2019

View reviewed changes

src/dogroups.c Show resolved Hide resolved

MichaelChirico changed the title ~~WIP: initial take at #3639 -- proper handling of CPLXSXP in dogroups~~ Closes #3639 -- proper handling of CPLXSXP in dogroups Jul 10, 2019

another switch() branch included

6ae6639

MichaelChirico commented Jul 10, 2019

View reviewed changes

src/dogroups.c Outdated Show resolved Hide resolved

MichaelChirico mentioned this pull request Jul 10, 2019

Master list: Feature completion for complex-valued columns #3690

Closed

11 tasks

Michael Chirico added 2 commits July 11, 2019 19:14

add fix&tests for assigning complex values (by subset or by group)

5a35c71

forgot to set seed for test

34fec0c

MichaelChirico commented Jul 11, 2019

View reviewed changes

Michael Chirico added 2 commits July 12, 2019 00:57

more tests

781df7f

overwrote variable in tests

d64f339

MichaelChirico commented Jul 11, 2019

View reviewed changes

coverage test

8967ee0

MichaelChirico commented Jul 12, 2019

View reviewed changes

Michael Chirico added 3 commits July 12, 2019 09:43

add expected output

d34989a

move to Rcomplex API

6bf80b1

Merge branch 'master' of https://github.com/Rdatatable/data.table int…

8565008

…o cplx_dogroups

MichaelChirico commented Jul 13, 2019

View reviewed changes

Merge branch 'master' into cplx_dogroups

4097e5f

mattdowle added this to the 1.12.4 milestone Jul 16, 2019

moved NA_CPLX to be defined once in .h and init.c

5258421

mattdowle merged commit 714e84f into master Jul 16, 2019

mattdowle deleted the cplx_dogroups branch July 16, 2019 22:37

MichaelChirico commented Jul 17, 2019

View reviewed changes

MichaelChirico mentioned this pull request Jul 17, 2019

forderv handles complex input #3701

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #3639 -- proper handling of CPLXSXP in dogroups #3689

Closes #3639 -- proper handling of CPLXSXP in dogroups #3689

MichaelChirico commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019

codecov bot commented Jul 10, 2019 •

edited

Loading

MichaelChirico commented Jul 10, 2019

MichaelChirico Jul 11, 2019

MichaelChirico Jul 11, 2019

MichaelChirico commented Jul 11, 2019 •

edited

Loading

MichaelChirico Jul 12, 2019

MichaelChirico Jul 12, 2019

mattdowle Jul 16, 2019 •

edited

Loading

MichaelChirico Jul 13, 2019

mattdowle Jul 16, 2019 •

edited

Loading

mattdowle commented Jul 16, 2019

eliocamp commented Jul 16, 2019

mattdowle commented Jul 16, 2019 •

edited

Loading

eliocamp commented Jul 16, 2019

mattdowle commented Jul 16, 2019

mattdowle commented Jul 16, 2019

MichaelChirico commented Jul 16, 2019

MichaelChirico Jul 17, 2019

mattdowle Jul 17, 2019

eliocamp commented Jul 17, 2019

mattdowle commented Jul 17, 2019

Closes #3639 -- proper handling of CPLXSXP in dogroups #3689

Closes #3639 -- proper handling of CPLXSXP in dogroups #3689

Conversation

MichaelChirico commented Jul 10, 2019 • edited Loading

codecov bot commented Jul 10, 2019

Codecov Report

codecov bot commented Jul 10, 2019 • edited Loading

Codecov Report

MichaelChirico commented Jul 10, 2019

MichaelChirico Jul 11, 2019

Choose a reason for hiding this comment

MichaelChirico Jul 11, 2019

Choose a reason for hiding this comment

MichaelChirico commented Jul 11, 2019 • edited Loading

MichaelChirico Jul 12, 2019

Choose a reason for hiding this comment

MichaelChirico Jul 12, 2019

Choose a reason for hiding this comment

mattdowle Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

MichaelChirico Jul 13, 2019

Choose a reason for hiding this comment

mattdowle Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

mattdowle commented Jul 16, 2019

eliocamp commented Jul 16, 2019

mattdowle commented Jul 16, 2019 • edited Loading

eliocamp commented Jul 16, 2019

mattdowle commented Jul 16, 2019

mattdowle commented Jul 16, 2019

MichaelChirico commented Jul 16, 2019

MichaelChirico Jul 17, 2019

Choose a reason for hiding this comment

mattdowle Jul 17, 2019

Choose a reason for hiding this comment

eliocamp commented Jul 17, 2019

mattdowle commented Jul 17, 2019

MichaelChirico commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019 •

edited

Loading

MichaelChirico commented Jul 11, 2019 •

edited

Loading

mattdowle Jul 16, 2019 •

edited

Loading

mattdowle Jul 16, 2019 •

edited

Loading

mattdowle commented Jul 16, 2019 •

edited

Loading