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

Support 64-bit integers in C on Windows #202

Closed
joshuaulrich opened this issue Jul 28, 2017 · 2 comments
Closed

Support 64-bit integers in C on Windows #202

joshuaulrich opened this issue Jul 28, 2017 · 2 comments
Assignees
Labels
enhancement Enhancement to existing feature

Comments

@joshuaulrich
Copy link
Owner

The main issue with the lack of 64-bit integers at the C level on Windows is that endpoints() does not support sub-second resolution. This is because the long C type is a 32-bit integer on Windows (even 64-bit Windows), but a 64-bit integer on widely-used unix-alikes.

library(xts)
x <- structure(1:3, .Dim = c(3L, 1L), index = structure(c(1501072999.073, 
  1501072999.178, 1501073000.136), tzone = "", tclass = c("POSIXct", "POSIXt")),
  .indexCLASS = c("POSIXct", "POSIXt"), .indexTZ = "", tclass = c("POSIXct", "POSIXt"),
  tzone = "", class = c("xts", "zoo"))
endpoints(x, "ms", 500)  # [1] 0 3 on Windows, but should be 0, 2, 3

The solution is to add #include <stdint.h> to any file that needs a 64-bit integer, and then use the C99 type int64_t. This may be an issue anywhere a cast to (long) is used.

@joshuaulrich joshuaulrich added the enhancement Enhancement to existing feature label Jul 28, 2017
@joshuaulrich joshuaulrich added this to the Release 0.10-1 milestone Jul 28, 2017
@joshuaulrich joshuaulrich self-assigned this Jul 28, 2017
joshuaulrich added a commit that referenced this issue Jul 29, 2017
The long C type is a 32-bit integer on Windows (even 64-bit Windows),
but a 64-bit integer on widely-used unix-alikes.  The C99 standard
includes a 64-bit integer, using the int64_t type defined in stdint.h.

Cast to int64_t instead of long when a 64-bit integer is needed.  Also
convert and rename real_tmp to int64_tmp to avoid a re-cast back to
double.  This should also make the logical comparisons more efficient.

See #202.
joshuaulrich added a commit that referenced this issue Jul 29, 2017
According to ?`%/%`:

'%%' and 'x %/% y' can be used for non-integer 'y', e.g. '1 %/% 0.2',
but the results are subject to representation error and so may be
platform-dependent.

The example below shows one consequence of this.  The 'x' object has
an observation every 100ms, so we expect endpoints(x, "ms", 200) to
produce a sequence from 0 to 10 in steps of 2.  But the 4th observation
is 7 instead of 6, the cause of which you can see from the output of
sprintf():

  R> x <- .xts(1:10, .POSIXct(1.5e9 + 0:9 / 10))
  R> endpoints(x, "ms", 200)
  [1]  0  2  4  7  8 10
  R> sprintf("%0.0f", .index(x) %/% 1e-3)
   [1] "1500000000000" "1500000000099" "1500000000200" "1500000000300"
   [5] "1500000000400" "1500000000500" "1500000000599" "1500000000700"
   [9] "1500000000800" "1500000000900"

The output is more reasonable if we convert to milliseconds and
microseconds by multiplying instead of integer division by a fraction.

  R> sprintf("%0.0f", .index(x) * 1e3)
   [1] "1500000000000" "1500000000100" "1500000000200" "1500000000300"
   [5] "1500000000400" "1500000000500" "1500000000600" "1500000000700"
   [9] "1500000000800" "1500000000900"

We don't need to worry about values to the right of the decimal place,
because the cast to 64-bit integer in the C code will truncate the
double values toward zero (according to C99).

See #202.
@joshuaulrich
Copy link
Owner Author

Closing as fixed, since commits 2665785 and bb08948 seem to address the bulk of the issue. There may still be some edge cases due to differences on other platforms.

@kboudt
Copy link
Sponsor

kboudt commented Aug 6, 2017

Thanks Joshua. This is a great improvement of the endpoints functionality, in particular for working with intraday price and quotes data with millisecond time stamp.

joshuaulrich added a commit that referenced this issue Sep 20, 2017
This restriction no longer applies, and should not be noted in the
documentation for endpoints().

See #202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to existing feature
Projects
None yet
Development

No branches or pull requests

2 participants