Skip to content

Added [<-.IDate so that rbind.data.frame retains integer IDate#3602

Merged
mattdowle merged 8 commits intomasterfrom
rbind_IDate
May 29, 2019
Merged

Added [<-.IDate so that rbind.data.frame retains integer IDate#3602
mattdowle merged 8 commits intomasterfrom
rbind_IDate

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented May 26, 2019

Closes #2008

I still don't really understand why these changes are necessary (and both are in fact necessary).

I posted this on r-devel, might be some insight there: https://stat.ethz.ch/pipermail/r-devel/2019-May/077866.html

@MichaelChirico
Copy link
Copy Markdown
Member Author

Josh Ulrich replied on r-devel, misunderstanding was pretty simple -- [<- is called by rbind.data.frame. Meanwhile [<-.Date does a funny thing where it retains its class while also calling as.Date, which converts to numeric.

So the approach here seems correct.

Comment thread inst/tests/tests.Rraw Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, the level of deja vu on reading this:

It seems that the .Internal rbind of two data.frame coerces IDate to numeric. Tried defining "[<-.IDate" as per Tom's suggestion, and c.IDate to no avail (maybe because the .Internal code in bind.c doesn't look up package methods?)

In fact I went through the same existential crisis for a while this morning! I'm not sure what was tried for [<-.IDate back then, maybe the NextMethod approach? I tried that as well but it seems fundamentally doomed because ... in NextMethod can't be passed as origin to the as.Date call in [<-.Date, which is needed if we pass the underlying integer to [<-.Date as value. If we pass an IDate, the #2008 problem happens -- double with IDate class. I didn't try running as.Date(value) but that would mean coercing numeric just to get [<-.Date and then back to integer aftewards... seems silly.

Anyway I think we can change this test that was testing that a bug was still a bug. Now the bug is fixed. I'm not sure if the subsequent test is still needed, but it can't hurt.

@codecov
Copy link
Copy Markdown

codecov bot commented May 26, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3602      +/-   ##
==========================================
+ Coverage   97.81%   97.81%   +<.01%     
==========================================
  Files          66       66              
  Lines       12909    12915       +6     
==========================================
+ Hits        12627    12633       +6     
  Misses        282      282
Impacted Files Coverage Δ
R/IDateTime.R 98.75% <100%> (+0.04%) ⬆️

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 5ceda0f...908701c. Read the comment docs.

typo
Comment thread NEWS.md Outdated
@mattdowle mattdowle added this to the 1.12.4 milestone May 29, 2019
@mattdowle mattdowle changed the title Closes #2008 -- updates for IDate such that rbind.data.frame retains … Added [<-.IDate so that rbind.data.frame retains integer IDate May 29, 2019
@mattdowle mattdowle merged commit 2d20594 into master May 29, 2019
@mattdowle mattdowle deleted the rbind_IDate branch May 29, 2019 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IDate storage mode not preserved in data frames

2 participants