Skip to content

between tzone to pass CoSMoS #3828

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.

Sign up for GitHub

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

Jump to bottom
Merged
mattdowle merged 4 commits into from
Sep 6, 2019
Merged

between tzone to pass CoSMoS #3828

mattdowle merged 4 commits into from
Sep 6, 2019

Conversation

@mattdowle
Copy link
Member

@mattdowle mattdowle commented Sep 6, 2019 •
edited
Loading

To resolve CoSMoS in #3581

require(CoSMoS)
example(analyzeTS)

WIP because although CoSMoS now gets past its between() call with the new message, comparing as UTC means nothing passes the subset and a later line then fails, as follows :

> ## Load data included in the package
> ## (to find out more about the data use ?precip)
> data('precip')
> ## Don't show: 
> ## test for one month to make it fast
> precip <- precip[between(date, as.POSIXct('1990-1-01', format('%Y-%m-%d'), tz = 'America/Regina'), as.POSIXct('1990-1-5', format('%Y-%m-%d'), tz = 'America/Regina'))]
'between' arguments are all POSIXct but have mismatched tzone attributes: ['', 'America/Regina', 'America/Regina']. The UTC times will be compared.
> a <- analyzeTS(precip)
Error in cor(x, y, use = "p") : 'x' is empty
Calls: analyzeTS ... seasonalACF -> lapply -> FUN -> sapply -> lapply -> FUN -> cor
Execution halted

Resolved. Need to run revdeps with TZ=UTC as CRAN does.

@mattdowle mattdowle added the WIP label Sep 6, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Sep 6, 2019
@mattdowle mattdowle mentioned this pull request Sep 6, 2019
Closed
31 tasks
@codecov
Copy link

codecov bot commented Sep 6, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3828      +/-   ##
==========================================
+ Coverage   99.42%   99.42%   +<.01%     
==========================================
  Files          71       71              
  Lines       13399    13403       +4     
==========================================
+ Hits        13322    13326       +4     
  Misses         77       77
Impacted Files Coverage Δ
R/between.R 100% <100%> (ø) ⬆️
R/utils.R 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 27f7516...7c84f81. Read the comment docs.

MichaelChirico
MichaelChirico reviewed Sep 6, 2019
View reviewed changes
@MichaelChirico
Copy link
Member

MichaelChirico commented Sep 6, 2019

a <- analyzeTS(precip) is not erroring for me with our between_tz and CRAN CoSMoS, are you on CoSMoS dev?

MichaelChirico
MichaelChirico reviewed Sep 6, 2019
View reviewed changes
@mattdowle
Copy link
Member Author

mattdowle commented Sep 6, 2019 •
edited
Loading

Passes for me with TZ=UTC now too. After this PR fix, the second problem depends on your local timezone. I had been running R CMD check under revdepsh without TZ=UTC set. run() at R level revdep.R level already set TZ=UTC. Good - all resolved then.

@mattdowle mattdowle removed the WIP label Sep 6, 2019
MichaelChirico
MichaelChirico reviewed Sep 6, 2019
View reviewed changes
MichaelChirico
MichaelChirico approved these changes Sep 6, 2019
View reviewed changes
@mattdowle mattdowle merged commit 4d355d6 into master Sep 6, 2019
@mattdowle mattdowle deleted the between_tz branch September 6, 2019 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelChirico MichaelChirico MichaelChirico approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.12.4

Development

Successfully merging this pull request may close these issues.

2 participants

@mattdowle @MichaelChirico