Implement push_back and push_front for DataFrame. #1099

waltersom · 2020-07-01T06:12:40Z

This explicitly defines push_back and push_back methods for DataFrame
which copies row.names and class attributes from the original object.

This fixes #1094 where these operations converted the object to a list.

Tests for this behavior are also implemented.

Checklist

Code compiles correctly
R CMD check still passes all tests
Prefereably, new tests were added which fail without the change
Document the changes by file in ChangeLog

waltersom · 2020-07-01T06:22:25Z

This does not enforce that the vectors are the same length (existing package doesn't either).

waltersom · 2020-07-01T07:52:57Z

The new version uses set__() in DataFrame.h, which has the huge benefit of padding/replicating vectors of different lengths, making names, and so forth.

waltersom · 2020-07-01T08:04:54Z

I still feel this is a bit inelegant as it only solves the problem for data frames, but I guess it is also class-specific how attributes should change when they are combined, so it might be better that any class that has a problem implements the appropriate fix like this. I'm not sure if any other classes suffer from this problem.

eddelbuettel · 2020-07-01T13:31:32Z

How does anybody else feel about changing the names of these verbs? push_back et al just invoked appending to a single vector. Maybe append_column and prepend_column (both clumsy) or add_col_at_back and add_col_at_front? Maybe with column instead of col ?

waltersom · 2020-07-01T19:50:19Z

push_* does sound a bit clumsy, but I am a bit concerned that it is possible to combine two data frames this way (or add a list to a data frame, I guess), and not just add a column. Would it work with just append and prepend?

kevinushey · 2020-07-01T20:54:24Z

My thoughts:

We should keep push_back() / pop_back(), to conform with the existing Vector interface.
If we feel it's worth it, we could add append_column() / prepend_column() as aliases for those functions. I don't have strong feelings here though.

I still feel this is a bit inelegant as it only solves the problem for data frames, but I guess it is also class-specific how attributes should change when they are combined, so it might be better that any class that has a problem implements the appropriate fix like this. I'm not sure if any other classes suffer from this problem.

I agree it'd be nice to fix this in general, but it seems hard to find the right solution that would work "in general" so it's okay to keep things scoped more narrowly here for now.

eddelbuettel · 2020-07-01T21:24:35Z

Happy to be majority-vote overruled but I still feel quite strongly that push_back() is not a good idea. It (to me) has clear connotations of extending a single vector ("verticallly" if you wish) whereas the PR uses to expand a list holding such vectors by enlarging a `data.frame ("horizontally"). Nothing good can come from mixing terminology that way.

kevinushey · 2020-07-01T21:50:19Z

My main concern is that DataFrame.push_back() already does exist (by virtue of being available in the base class) and so I don't think we should change the pre-existing semantics there.

If this were a clean-room rebuild of the class, I would definitely be on board though.

eddelbuettel · 2020-07-01T22:12:19Z

I agreee with that point @kevinushey. That is indeed more important. Having the aliases is then a good compromise.

eddelbuettel · 2020-07-06T17:24:37Z

Thanks for your patience. Rcpp 1.0.5 is now on CRAN, I am currently looking into #1098 and will turn to this afterwards. Likely in a day or two.

This explicitly defines push_back and push_back methods for DataFrame which copies row.names and class attributes from the original object. This fixes #1094 where these operations converted the object to a list. Tests for this behavior are also implemented.

Make use of the fact set__() already does a conversion, and just use that. This has the benefit that it works with push_back(DataFrame), as well as push_back(Vector), and also handles adding vectors of different lengths properly.

eddelbuettel · 2020-07-07T20:21:39Z

I rebased this relative to the updated main branch and pushed the update. I have a local (not-yet committed) change adding ChangeLog and setting this as 1.0.5.2 and am now running reverse depends. We will know more by tomorrow this time.

eddelbuettel · 2020-07-08T21:12:54Z

Hi @waltersom and apologies for taking a moment---but I first wanted to get 1.0.5 out of the door, and then test the PR by @kevinushey which came in just before yours.

I have now done a full run, and in this case the full test paid us back. Package lidR and metacart fail under your PR. Both show a message of 'Could not convert using R function: as.data.frame.'. Could you take another look at the PR?

Many thanks!

waltersom · 2020-07-08T21:33:06Z

No worries about the time, it's good to get it right, and there's no great rush.

I'll have a look at why they might get that error.

waltersom · 2020-07-09T01:42:22Z

Ok, I have some minimum examples which exhibit these problems; f is representative of the problem in lidR, and g from metacart.

library(Rcpp)

f <- cppFunction('DataFrame d(){
DataFrame d = DataFrame::create();

NumericVector v1(5);
NumericVector v2(5);
DataFrame d1 = DataFrame::create(_["a"]=v1, _["b"] = v2);
d.push_back(v1);
SETLENGTH(wrap(v1), 2);
d.push_back(v2);
return d;
}')
f()
#> Error in f(): Could not convert using R function: as.data.frame.


g <- cppFunction('DataFrame d(){
DataFrame d = DataFrame::create();

NumericVector v1(5);
NumericVector v2(6);
d.push_back(v1);
d.push_back(v2);
return d;
}')
g()
#> Error in g(): Could not convert using R function: as.data.frame.

^{Created on 2020-07-09 by the reprex package (v0.3.0)}

Using the pre-PR code, I get

library(Rcpp)

f <- cppFunction('DataFrame d(){
DataFrame d = DataFrame::create();

NumericVector v1(5);
NumericVector v2(5);
DataFrame d1 = DataFrame::create(_["a"]=v1, _["b"] = v2);
d.push_back(v1);
SETLENGTH(wrap(v1), 2);
d.push_back(v2);
return d;
}')
f()
#> [[1]]
#> [1] 0 0
#> 
#> [[2]]
#> [1] 0 0 0 0 0
as.data.frame(f())
#> Error in (function (..., row.names = NULL, check.rows = FALSE, check.names = TRUE, : arguments imply differing number of rows: 2, 5

g <- cppFunction('DataFrame d(){
DataFrame d = DataFrame::create();

NumericVector v1(5);
NumericVector v2(6);
d.push_back(v1);
d.push_back(v2);
return d;
}')
g()
#> [[1]]
#> [1] 0 0 0 0 0
#> 
#> [[2]]
#> [1] 0 0 0 0 0 0
as.data.frame(g())
#> Error in (function (..., row.names = NULL, check.rows = FALSE, check.names = TRUE, : arguments imply differing number of rows: 5, 6

^{Created on 2020-07-09 by the reprex package (v0.3.0)}

My feeling is that both packages are constructing invalid dataframes, and probably getting them fixed would be good.

The fix for both is easy, and just involves changing a DataFrame to a List (which is essentially what it was before, after a push_back).

waltersom · 2020-07-09T01:46:43Z

The relevant lines are (eg) https://github.com/Jean-Romain/lidR/blob/9f30c6f8065b6fe6571c2d53457cfbab4a1fc3ca/src/LAS.cpp#L1328-L1331
and
https://github.com/cran/metacart/blob/master/src/testxvalid.cpp#L33-L53

eddelbuettel · 2020-07-09T01:47:21Z

My feeling is that both packages are constructing invalid dataframes

That is possible but we would probably have to find their code [and as I type this you post it, will study.]

waltersom · 2020-07-09T01:48:48Z

@eddelbuettel I have a fix for lidR, will get one for metacart as well.

eddelbuettel · 2020-07-09T01:49:48Z

Do you have a handle on why their length differ between columns? That seems unlikely to be on purpose.

[ And I don't recall from the top of my head whether we "help" by recycling, my instinct is to say we do not. ]

The number of unique elements in different columns in x1 may be different, which leads to attempting to add vectors of different length into res. Previously, `push_back` on a DataFrame changed the DataFrame to a list, which is being addressed in RcppCore/Rcpp#1099. This change, combined with attempting to add vectors of different lengths, leads to an error.

waltersom · 2020-07-09T02:45:47Z

My fixes are at https://github.com/waltersom/lidR/tree/replace-dataframe-with-list and https://github.com/waltersom/metacart/tree/replace-dataframe-list.

For lidR, they don't even return the DataFrame/list, it's just a convenience, see
https://github.com/Jean-Romain/lidR/blob/9f30c6f8065b6fe6571c2d53457cfbab4a1fc3ca/src/LAS.cpp#L1214-L1215
I have to admit I don't really understand it, and it works most of the time, but there are occasions (which cause the test to fail) where they shorten a vector after adding it to the DataFrame, and it's already in another DataFrame. If we stick to a list, it works.

For metacart, I understand it even less. But the examples from ?FEmrt and ?REmrt run with this PR, and my fix to metacart.

Our push_* (as from as.data.frame) doesn't help by recycling, unless the length of a vector is 1.

eddelbuettel · 2020-07-09T02:50:27Z

Nice work. I can't look at it end -- end of day / evening here. There usual process is to propose a change to the affected package to let it build with the existing (i.e. 1.0.5) and proposed Rcpp (where it currently only works with existing). That is usually done by email to them, politely asking for a fix in two weeks or so. Once done we can change Rcpp itself.

You've done so much already so let me be outrageous and ask directly: Would you be able to take care of preparing the patches and emails, i.e. drive all that from your end?

waltersom · 2020-07-09T03:00:51Z

I was already preparing a PR for lidR (r-lidar/lidR#358), and am about to look at metacart next.

In both cases, the changes should work both with or without the change to Rcpp. It is only that once this (#1099) is merged, they break without the changes.

eddelbuettel · 2020-07-09T03:07:01Z

You put that well to lidR in its 357 and 358. The fact is that Rcpp::DataFrame is not one of our strongest classes and I personally have always grown lists---precisely what you propose here---because DataFrame is half baked. Your PR will make it better so this kicks a few cans down the right road!

The number of unique elements in different columns in x1 may be different, which leads to attempting to add vectors of different length into res. Previously, `push_back` on a DataFrame changed the DataFrame to a list, which is being addressed in RcppCore/Rcpp#1099. This change, combined with attempting to add vectors of different lengths, leads to an error.

waltersom · 2020-07-09T03:29:21Z

I have also emailed the metacart maintainer, so hopefully at some point soon those two packages will be updated.

…

On Thu, 9 Jul 2020 at 15:07, Dirk Eddelbuettel ***@***.***> wrote: You put that well to lidR in its 357 and 358. The fact is that Rcpp::DataFrame is *not* one of our strongest classes and I personally have always grown lists---precisely what you propose here---because DataFrame is half baked. Your PR will make it better so this kicks a few cans down the right road! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1099 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7W2M5BEJR5HSGU5JGFS6DR2UX6DANCNFSM4ONCHN6Q> .

waltersom · 2020-07-09T06:06:33Z

One thing these packages failing highlights is that the error message when attempting to construct an invalid DataFrame is not very helpful.
It would be possible to check if vector of different (non-1) length are being added, and either print a more helpful error, or not call set__() (and thus revert to previous behaviour). There would be some (small) overhead from doing this, and it's not very elegant, but if others thought it would be helpful, I could add that.

eddelbuettel · 2020-07-09T10:52:35Z

A warning() or error() is probably a good idea.

Food for thought: maybe make it even more complex and add an option() to fall back to old behavior? Or do we think that is a bad idea, and that we should enforce the same length now?

With this commit, if after a push_back the row count of the DataFrame would be invalid, a warning is generated. The type is then not coerced to a DataFrame, but left as a list. In future, this could instead `stop()`.

eddelbuettel

LGTM!

Edit: Whoops, mean to comment/approve the last commit only. Oh well ;-)

waltersom · 2020-07-09T13:15:49Z

I have implemented a function set_type_after_push(), which checks if any columns have a length that is different from 1, the maximum number of rows, or an integer factor of the max. In such a case, it doesn't call set__() (so wouldn't trigger an error), and does call warning(). It might be preferable at some point to change to stop().

But, thinking about it, let me check what happens if one of the columns has zero length...

eddelbuettel · 2020-07-09T13:18:32Z

Actually, using warning() is good because there is an existing hok for 'warnings-as-errors' that already gives us the stop().

eddelbuettel · 2020-07-09T13:26:40Z

I guess you noticed

 Error: ----- FAILED[data]: test_dataframe.R<112--112>
   call| expect_equal(DataFrame_PushReplicateLength(), df)
   diff| Names: 2 string mismatches 
  Execution halted

eddelbuettel · 2020-07-09T13:27:42Z

ChangeLog

@@ -17,6 +17,13 @@
        * inst/NEWS.Rd: Idem
        * vignettes/rmd/Rcpp.bib: Idem
        * inst/bib/Rcpp.bib: Idem
+2020-07-01 Walter Somerville <waltersom@gmail.com>


This should be

in chronological order

with proper empty lines

I just sent a trivial commit back to you for that so make sure you pull to your box.

This triggers the warning when adding a zero-length object to a DataFrame. It is not a valid DataFrame, so calls `warning()`, and doesn't call `as.data.frame()`.

waltersom · 2020-07-09T13:48:27Z

Ok, I have corrected that failing test. Thanks for doing the changelog.

Jean-Romain · 2020-07-09T13:55:41Z

lidR maintainer here. The fix has been merged in master. The use of DataFrame was just a convenience to highlight the fact that all elements are the same length but the use of a List is valid as well.

Please keep me updated when you plan to release this change. I won't release a new version of lidR right now because I already made several releases recently. When you will be ready for a release of Rcpp I will release lidR ahead of time so this change won't break reverse dependency.

eddelbuettel · 2020-07-09T13:57:38Z

Salut Jean-Romain

It works the other way around. We cannot upload until you upload as our now-stricter-in-this-PR code breaks your package, and CRAN will not admit "breaking" packages. We are not in a great hurry as we just released, and but the point also is to not have this linger. Could you update within, say, two weeks which is the common timeframe CRAN itself often gives?

Jean-Romain · 2020-07-09T14:09:34Z

I know it works the other way. This is why I suggested to tell me when you plan to release Rcpp so I can release my package first. I believed that the common time-frame given by the CRAN was more like 1 month but I can release in two weeks if mandatory.

eddelbuettel · 2020-07-09T14:13:09Z

Rcpp will likely not have a release for many months [1], but that is no reason to push this off "forever". I apologize if we were unclear, we did not mean to put a gun to your head and scream "update NOW" but as we depend upon your cooperation here it would help us if you did it "sooner rather than later". Again, it does obviously not have to be this week. But everybody is busy and it helps to commit to a) action and b) a date. So if you could say "sure, but this is summer so give me three weeks" it would help us too.

[1] But planning is one thing. The possible need for a hot-fix bug release is always a possibility too.

Jean-Romain · 2020-07-09T14:21:47Z

No problem. It will definitively be done within a month maximum and I won't delay that forever.

eddelbuettel · 2020-07-09T14:24:44Z

@waltersom the last commits of yours and mine got out of sync so you needed a merge -- I just cleaned that up with squash rebase and force-push. To catch you would need to reset your end a few commits and then pull.

eddelbuettel · 2020-07-09T14:25:21Z

@Jean-Romain Thanks. If and when you do please let us know here so that we can proceed too.

waltersom · 2020-07-10T11:38:13Z

I have just heard from the maintainer of metacart that he has submitted a new version to CRAN that won't fail with our changes.

…

On Fri, Jul 10, 2020, 02:25 Dirk Eddelbuettel ***@***.***> wrote: @Jean-Romain <https://github.com/Jean-Romain> Thanks. If and when you do please let us know here so that we can proceed too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1099 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7W2M6FOHZEOGTMK4RZVMLR2XHODANCNFSM4ONCHN6Q> .

eddelbuettel · 2020-07-10T21:14:09Z

Just tested metacart 2.0-3, already on CRAN, and as expected it passes. Nice work!

eddelbuettel · 2020-08-01T11:33:02Z

Hi @Jean-Romain

The last lidR release on CRAN was on July 5, which will be a month ago in a few days. Given that CRAN will close for the summer between August 14 and 24, I would really appreciate it if you could update lidR later this week. Respecting CRAN's wishes for updates only about once a month is important, but it would also be nice if you could help us let Rcpp move forward with the change from this PR.

Jean-Romain · 2020-08-03T10:14:18Z

The new version is now on CRAN.

eddelbuettel · 2020-08-03T11:42:15Z

@Jean-Romain Thank you! I will proceed with Rcpp tests and changes then clearing the way for this PR.

eddelbuettel · 2020-08-04T23:05:00Z

Thanks everybody for their patience, of course @waltersom for writing the PR in the first place, and to @jclaramunt for very promptly updating metacart, and to @Jean-Romain for doing the same with lidR (letting some time pass since its last update).

Reverse depends checks revealed no issue so this now goes into the main branch. Yay!!! And again, thanks to all for help!

eddelbuettel · 2021-01-15T14:01:20Z

Thanks again for everybody's patience. This is now 'live' in Rcpp 1.0.6 which just got to CRAN under our bi-annual release cycle.

waltersom added 3 commits July 7, 2020 06:51

Correct format of ChangeLog entry

d37dae3

Modify approach to copying attributes.

fab8b38

Make use of the fact set__() already does a conversion, and just use that. This has the benefit that it works with push_back(DataFrame), as well as push_back(Vector), and also handles adding vectors of different lengths properly.

This was referenced Jul 9, 2020

Use a List for proxy, instead of DataFrame r-lidar/lidR#357

Closed

Replace DataFrame with List. r-lidar/lidR#358

Merged

Add in warning when DataFrame created will have incorrect rows.

d800e2a

With this commit, if after a push_back the row count of the DataFrame would be invalid, a warning is generated. The type is then not coerced to a DataFrame, but left as a list. In future, this could instead `stop()`.

eddelbuettel approved these changes Jul 9, 2020

View reviewed changes

eddelbuettel reviewed Jul 9, 2020

View reviewed changes

waltersom added 2 commits July 10, 2020 01:31

Correct behaviour when pushing zero-length object to DataFrame.

2bfd0d1

This triggers the warning when adding a zero-length object to a DataFrame. It is not a valid DataFrame, so calls `warning()`, and doesn't call `as.data.frame()`.

Correct failing test.

89b69dd

correct ChangeLog

ff14167

eddelbuettel merged commit 05eea3d into RcppCore:master Aug 4, 2020

Implement push_back and push_front for DataFrame. #1099

Implement push_back and push_front for DataFrame. #1099

Conversation

waltersom commented Jul 1, 2020

Checklist

waltersom commented Jul 1, 2020

waltersom commented Jul 1, 2020

waltersom commented Jul 1, 2020

eddelbuettel commented Jul 1, 2020

waltersom commented Jul 1, 2020

kevinushey commented Jul 1, 2020

eddelbuettel commented Jul 1, 2020

kevinushey commented Jul 1, 2020

eddelbuettel commented Jul 1, 2020

eddelbuettel commented Jul 6, 2020

eddelbuettel commented Jul 7, 2020

eddelbuettel commented Jul 8, 2020

waltersom commented Jul 8, 2020

waltersom commented Jul 9, 2020

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

waltersom commented Jul 9, 2020 via email

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

eddelbuettel left a comment • edited Loading

Choose a reason for hiding this comment

waltersom commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

eddelbuettel Jul 9, 2020

Choose a reason for hiding this comment

eddelbuettel Jul 9, 2020

Choose a reason for hiding this comment

waltersom commented Jul 9, 2020

Jean-Romain commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

Jean-Romain commented Jul 9, 2020 • edited Loading

eddelbuettel commented Jul 9, 2020 • edited Loading

Jean-Romain commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

eddelbuettel commented Jul 9, 2020

waltersom commented Jul 10, 2020 via email

eddelbuettel commented Jul 10, 2020 • edited Loading

eddelbuettel commented Aug 1, 2020

Jean-Romain commented Aug 3, 2020

eddelbuettel commented Aug 3, 2020

eddelbuettel commented Aug 4, 2020

eddelbuettel commented Jan 15, 2021

eddelbuettel left a comment •

edited

Loading

Jean-Romain commented Jul 9, 2020 •

edited

Loading

eddelbuettel commented Jul 9, 2020 •

edited

Loading

eddelbuettel commented Jul 10, 2020 •

edited

Loading