Time bug fix #165
Time bug fix #165
Conversation
The new anonymised ccRecords were causing R to crash when using create_cctable(). This is because the findMaxTime function was returning the number of hours since 1970 and forcing vector allocation to this amount. This fixes the issue
|
Not sure. I'll check with Roma a little later on today |
further conditional logic added to account for null return from findMaxTime()
| @@ -3,3 +3,5 @@ Makefile | |||
| .travis.yml | |||
| appveyor.yml | |||
| paper.md | |||
| ^.*\.Rproj$ | |||
| ^\.Rproj\.user$ | |||
There was a problem hiding this comment.
You don't need to add these to the PR. It should just be the things needed to fix the issue you are worried about.
| @@ -24,3 +24,5 @@ vignettes/*.pdf | |||
|
|
|||
| # OAuth2 token, see https://github.com/hadley/httr/releases/tag/v0.3 | |||
| *.httr-oauth | |||
| .Rproj.user | |||
| *.Rproj | |||
There was a problem hiding this comment.
You wouldn't normally add these to the git repo either. There is a tendency to put IDE / editor specific ignore stuff in your system gitignore to prevent it from getting too crazy. That said, this has the same, you probably wouldn't add it to a PR as it isn't relevant property as above.
| @@ -466,6 +474,8 @@ getEpisodePeriod <- function (e, unit="hours") { | |||
| if (period_length == 0) | |||
| period_length <- period_length + 1 | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Why are you adding two blank lines here? What are they for?
| if (is.na(tadm) || is.na(tdisc)) | ||
| period_length <- findMaxTime(e) | ||
| else { | ||
| if (is.na(tdisc)) { |
There was a problem hiding this comment.
tadm can probably be NA in general. It is only a specific property of the dataset you have that it cannot.
| as.POSIXct.numeric(findMaxTime(e), origin = "1970-01-01 00:00:00"), tadm, units = unit) | ||
| )) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Why are you finding max time twice?
|
The failing tests are actually things which are using |
This segment of the code caused a problem with allocation of memory.
It looks like
findMaxTime()was the culprit returning a very high number (possibly the number of hours since 1900??)This modification works, but may effect legacy
ccRecordsthat were created outside the Postgres pipeline. I'm not very familiar with that aspect, so this needs review in the context of preserving legacy features.I have removed reference to
is.na(tadm)because this field is always present in the new system (A record can't exist without an admission time)Many thanks in advance for input.