Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix round.IDate weeks #4334

Closed
wants to merge 747 commits into from
Closed

Conversation

artemklevtsov
Copy link

@artemklevtsov artemklevtsov commented Apr 1, 2020

Related issue: #4335

round.IDate for the weeks (IDateTime.R#81) seems inaccurate: first week of year have a 6 days instead 7 (fix: should be yday(x) - 1L):

> rle(unclass(fun_old(as.IDate(0:21), "week")))
Run Length Encoding
  lengths: int [1:4] 6 7 7 2
  values : int [1:4] 0 7 14 21
> rle(unclass(fun_new(as.IDate(0:21), "week")))
Run Length Encoding
  lengths: int [1:4] 7 7 7 1
  values : int [1:4] 0 7 14 21

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.61%. Comparing base (cbacb9e) to head (cbacb9e).
Report is 749 commits behind head on master.

❗ Current head cbacb9e differs from pull request most recent head 427db7a. Consider uploading reports for the commit 427db7a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4334   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          72       72           
  Lines       13916    13916           
=======================================
  Hits        13862    13862           
  Misses         54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@artemklevtsov artemklevtsov mentioned this pull request Jan 9, 2021
artemklevtsov added a commit to artemklevtsov/data.table that referenced this pull request Jan 9, 2021
mattdowle and others added 27 commits November 2, 2022 01:03
…ata.table() call that 1538 in Rdatatable#5520 bumped up against; perhaps combined with object.size. Anyway, both now improved.
…topped adding commas to NCOL, NROW and MB columns, and more
…ve #if up to data.table.h thanks Jan, plotting in dev memtest need not require imports of standard functions (strange)
…and bumped dev to 1.14.7. Will tag patch-1.14 branch when publish date is confirmed in case there are revisions.
…r tag); Makefile & CRAN_Release version number bump
MichaelChirico and others added 20 commits March 7, 2024 12:04
* Better pipeline failure when curl fails on Windows

* stylistic changes
* add fix for subset

* add NEWS

---------

Co-authored-by: Benjamin Schwendinger <benjamin.schwendinger@tuwien.ac.at>
…ntainability (Rdatatable#5988)

* Changed blanket imports to selective imports

* removed quotes

* alphabetize

* removed time from imports

* Update test.data.table.R

* added name to Authors@R

---------

Co-authored-by: nitish jha <nitishjha@nitishs-MacBook-Air.local>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
…datatable#5986)

* fix test relying on deprecated behavior comparing language objects

* NEWS
* check if builtPath exists

* fix builtPath

* update dev README

* make file.path platform independent

Co-authored-by: Michael Chirico <chiricom@google.com>

* soften language around the use of cc()

* cache r versino

* clarify dev README

* update comment

* rename variable for consistency

---------

Co-authored-by: Michael Chirico <chiricom@google.com>
Co-authored-by: Tyson Barrett <t.barrett@aggiemail.usu.edu>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* Add options= to test()

document in Rd

* Add options= to test()

document in Rd

* missed staged chunk
* delete old commented code

* new test for no warning fails

* only compute default fill if missing cells present

* any_NA_int helper

* bugfix Rdatatable#5512

* Update src/fcast.c

Co-authored-by: Xianying Tan <shrektan@126.com>

* Update src/fcast.c

Co-authored-by: Xianying Tan <shrektan@126.com>

* mention warning text

* const int args

* add back ithiscol

* get pointer before for loop

* add test case from Michael

* test min(dbl) and no warning when fill specified

* Revert "delete old commented code"

This reverts commit 2886c4f.

* use suggestions from Michael

* rm inline any_NA_int since that causes install to fail

* clarify comment

* link 5390

* mymin test fails

* compute some_fill using anyNA in R then pass to C

* Update R/fcast.R

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Update R/fcast.R

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* dat_for_default_fill is zero-row dt

* !length instead of length==0

* new dcast tests with fill=character

* dat_for_default_fill is dat again, not 0-row, because that causes some test failure

---------

Co-authored-by: Xianying Tan <shrektan@126.com>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
…thods) (Rdatatable#6001)

* need importClassesFrom

* revert methods:: change, not necessary
…datatable#5364)

* fwrite(..., row.names=TRUE) print row.names instead of row numbers

* add issue comment to tests + code

* restore NEWS notes

* spare [

* Update NEWS.md

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Update R/fwrite.R

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* improve test readability\nalso switched test order to make it easier

* ... not needed

* data.table style

* One more local var to focus on what changes between tests

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* Document data.tables with no columns

* Mention that rows are children of columns

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
…type (Rdatatable#5805)

* add feature

* change fill

* undup code

* update arguments

* add man

* add tests

* update usage docs

* add coverage

* add factors test

* update tests for factors

* add NEWS

* update news

* add example to docs

* update docs

* Update NEWS.md

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* remove extra blank line

* ease t/f error

* rm blank line

* restore logical case

* reordering test case numbers

* fix LGL case

* use unlist as proper action

* move NEWS

* fix doc

* rm blank line in tests

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
…e#6005)

* Documentation for char.trunc('datatable.prettyprint.char')

* small fix

* Mentioned trunc.char in 'print.data.table' function

* Improved explanation

* Update man/print.data.table.Rd

Co-authored-by: Joshua Wu <124658199+joshhwuu@users.noreply.github.com>

* Update man/print.data.table.Rd

Co-authored-by: Joshua Wu <124658199+joshhwuu@users.noreply.github.com>

* Update man/print.data.table.Rd

Co-authored-by: Joshua Wu <124658199+joshhwuu@users.noreply.github.com>

* added description, deleted from alias, usage, args

* final touches

* removed name of private function

* removing space

* Final tweaks

---------

Co-authored-by: nitish jha <nitishjha@nitishs-MacBook-Air.local>
Co-authored-by: Joshua Wu <124658199+joshhwuu@users.noreply.github.com>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* better test failure with non-ASCII characters

* intentional fail to check output on Windows

* revert to merge

* Update test.data.table.R
Co-authored-by: nitish jha <nitishjha@nitishs-MacBook-Air.local>
Rdatatable#6012)

* added tests for DT[, .SD] retaining secondary indices, Rdatatable#1709

* updated news.md

* NEWS not needed

* terminal newline

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* Update data.table.R

* Update tests.Rraw

* Update data.table.R

* Update tests.Rraw

* Update datatable-reference-semantics.Rmd

* Update assign.Rd

* Update NEWS.md

* Update NEWS.md

* Update data.table.R

* Update tests.Rraw

* Update tests.Rraw

* Update data.table.R

* Update tests.Rraw

* replace iris with raw dataset

* Update tests.Rraw

* update replace_names_sd and made .SD := not work

* change .SD to names(.SD)

* update typo; change .SD to names(.SD)

* update to names(.SD)

* include names(.SD) and fx to .SD usage

I may have went too far. There's no use of ```(cols) := ...``` now but there is at least a reference to the other vignette.

* Updates news to names(.SD)

* Update typo.

* tweak NEWS

* minor grammar

* jans comment

* jan's comment (ii)

* added "footnote"

* Add is.name(e[[2L]])

* Put tests above Add new tests here

* added test to test names(.SD(2))

* include .SDcols in example for assign

* included .SDcols = function example

* test 2138 is greater than 2137

* bad merge

* Make updates per Michael's comments.

* Added test where .SD is used as well as some columns not in .SD.

* Mention count of reactions in issue

* small copy-edit

* more specific

* specify LHS/RHS

* Simplify implementation to probe for names(.SD) and new test

* fine-tune comment

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
…of new column (Rdatatable#6016)

* Updated warning message in assign.c, as well as updated tests 316, 944.1 and 944.3

* added spaces for consistency

* Update src/assign.c warning

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* updated tests

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* fread: turn off sampling for fill

* fixed stop

* add stopf

* fread: turn off sampling for fill

* added coverage

* coverage

* revert additional argument

* fill upperbound

* integer as fill argument

* fix typo

* fix L

* add NEWS

* update verbose

* undo verbose

* init cleanup

* fix typo news

* renum NEWS

* add proper cleanup of overallocated columns

* add tests and coverage

* fix tests

* add tests

* cleanup

* update NEWS

* update tests

* Refine NEWS

* use integer for fill

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* refine warning

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* wording

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* test readability

* small tweak to NEWS

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
* Add options= to test(), convert most Rraw scripts

data.table spacing style

document in Rd

Add options= to test()

document in Rd

missed staged chunk

convert most Rraw scripts to use test(options=)

Merge branch 'master' into test-options

Merge remote-tracking branch 'origin/test-options' into test-options

* trailing ws
… Environment (Rdatatable#6019)

* adding error for when := is called in not data.table aware enviroment

* added tests

* whitespace

* Update R/data.table.R

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Update tests.Rraw

* sprintf() won't coerce symbol->character, do it explicitly

* cleanup of test, better error message test

---------

Co-authored-by: nitish jha <nitishjha@nitishs-MacBook-Air.local>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
@MichaelChirico
Copy link
Member

Looked around for some guidance on how else this is implemented, came across lubridate::round_date(). They solve the problem I noted above by adding a second parameter, week_start, to govern, under round(unit="week"), to which day of the week to round. Maybe that's the most sensible approach.

@MichaelChirico
Copy link
Member

Hmm things seem to have been borked by trying to update the branch. Not sure what happened / how to fix it. Part of the issue is likely that the change was created on master in the fork, which makes things harder to work with even though we have write permissions. Note that it's much preferable to make work on branches other than master. Closing this and resuming work in #6024

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.

None yet