Skip to content

between NA bounds, nanotime support#3731

Merged
mattdowle merged 17 commits intomasterfrom
between-char-NA-bounds
Aug 23, 2019
Merged

between NA bounds, nanotime support#3731
mattdowle merged 17 commits intomasterfrom
between-char-NA-bounds

Conversation

@jangorecki
Copy link
Copy Markdown
Member

@jangorecki jangorecki commented Jul 30, 2019

closes #3667
For upper/lower argument of vector type (non-scalar) could be better optimised. IMO it is not a priority as we ultimately need to push down character handling into C. Which was also mentioned in comment

TODO: support character between in Cbetween and remove this branch

This PR at least put expected functionality and unit tests in place so when we proceed to C development we have a properly working R code to match to.


also closes #3522 by fallback to R code


also added nanotime support

@jangorecki jangorecki changed the title Between char na bounds Between char NA bounds Jul 30, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 30, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3731      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          71       71              
  Lines       13199    13210      +11     
==========================================
+ Hits        13122    13133      +11     
  Misses         77       77
Impacted Files Coverage Δ
src/utils.c 100% <100%> (ø) ⬆️
R/between.R 100% <100%> (ø) ⬆️
src/init.c 100% <100%> (ø) ⬆️
src/between.c 100% <100%> (ø) ⬆️

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 a8e926a...3bf1d02. Read the comment docs.

@jangorecki jangorecki changed the title Between char NA bounds between NA bounds Jul 30, 2019
@jangorecki jangorecki changed the title between NA bounds between NA bounds, nanotime support Aug 3, 2019
@jangorecki
Copy link
Copy Markdown
Member Author

jangorecki commented Aug 3, 2019

failure of CI is quite interesting.
github is automerging this branch into master, and CI is testing some artificial merge commit: ea7d608
at first glance it looks like a bug
gitlab CI handles that as expected: https://gitlab.com/jangorecki/data.table/pipelines/74562885

@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Aug 23, 2019

This one was tricky! Changes I made were

  • NAbounds implemented at C level
  • Supported character at C level
  • Changed new NAbounds= from TRUE/FALSE to TRUE/NA. The idea being that TRUE/FALSE is hard to remember which way around it means. But if you remember it as what is returned for that bound when there's an NA, then TRUE/NA hopefully is easier to remember. Explained in the news item and man page too, using the adjective 'unlimited' bound.
  • Rinherits() internal C function created which observes S4 inheritance from integer64 (nanotime). Enabled removing special cases for nanotime. Might be able to remove nanotime special cases elsewhere too now using this.
  • Moved INHERITS to utils.c to be next to Rinherits(). I'm not comfortable with their names (e.g. all-caps doesn't give much of clue) but I couldn't think what would be better names right now. They are internal functions so we can change their names anytime.

@mattdowle mattdowle added this to the 1.12.4 milestone Aug 23, 2019
@mattdowle mattdowle merged commit bfca73d into master Aug 23, 2019
@mattdowle mattdowle deleted the between-char-NA-bounds branch August 23, 2019 22:55
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.

Different %between% behavior on character vectors undocumented between changed behaviour from NA bounds 'unknown' to 'missing'

2 participants