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

add sanity checks for bbgDateToJulianDate #142

Merged
merged 2 commits into from Mar 2, 2016

Conversation

@armstrtw
Copy link
Contributor

@armstrtw armstrtw commented Mar 2, 2016

given how poor bbg data integrity is, I think we need these checks in place.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

Sure. Checks are good.

One Q for you though: do we actually really want Julian dates or was that just a way to get dates as ints? Rcpp has (still poor, to be rewritten) Rcpp::Date around R's date, and we do of course have Boost Date_Time.

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

not sure I'm following you.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

Why have Julian date when we can have a native Date type?

Why translate into a scheme nobody uses (Julian) when we can have one humans and computers can use as easily?

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

but Julian is just R's date type, right (days since 1970-01-01)? what am I missing?

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

We are getting closer!

See Wikipedia for Julian Date or just help(julian) (and scroll to the end!!) to clear up some of your confusion.

R> today <- Sys.Date()
R> today
[1] "2016-03-02"
R> julian(today, -2440588)      # actual Julian day as defined
[1] 2457450
attr(,"origin")
[1] -2440588
R> as.integer(today)        # my preferred form, and what R's Date type uses internally
[1] 16862
R> 

Maybe you always uses the julian() function with the epoch as an offset --- highly nonstandard.

So I think we agree on what we should do -- store today, say, as 16862. We refer to it in two different ways, and yours may risk being misunderstood as it was by me :)

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

Ahh, yes. I think we agree. and funny that I found this:
http://whatis.techtarget.com/definition/Julian-date
2) Commonly in computer programming, Julian date has been corrupted to mean the number of elapsed days since the beginning of a particular year. For example, in this usage, the Julian date for the calendar date of 1998-02-28 would be day 59.

So, what I mean is that we should return the data as R's 'Date' class, or days since 1970-01-01.

If you look at the conversion function, all should be clear, and please let me know if you think we should return as a different type.
https://github.com/Rblp/Rblpapi/blob/feature-add-checks-for-julian-date-conversion/src/blpapi_utils.cpp#L70

and the new one for converting doubles:
https://github.com/Rblp/Rblpapi/blob/feature-add-checks-for-julian-date-conversion/src/blpapi_utils.cpp#L80

This patch doesn't change the return format from master. I just added checks to make sure BBG didn't give us a date with 'time' parts, which we would then ignore by converting to an R date.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

If you already returned 16862 then it is just a matter of renaming away the misleading Julian label :)

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

ok, you want me to rename the functions as part of this patch?

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

Is there a name for R's date format, other than just RDate or something similar?

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

In R, Date

In Rcpp, Rcpp::Date (pretty much a one-to-one map; just try it).

In R'c C API: I think INTSXP with class attribute, need to check.

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

Yes. for now we're using C API INTSXP.

Ok. for now I'm just renaming things. If we want to change the tooling, that's more of a major change and I would prefer to do it as a separate patch.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

Please don't.

My medium-term goal is to get rid of all SEXP use. This is a C++ project. Why not use the niceties such as ctor, dtor, exceptions, ... ?

Rcpp::Date is ready for you. RQuantLib has used it for a decade, also interchanging with Boost.

Happy to do cleanup but let's not add more SEXP line noise.

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

this is a patch to add error checking. switching tooling is outside the scope of this patch.

I'm happy to switch to more Rcpp as I have been doing in all the recent code I've added, and I can do as a separate patch, which I've already said.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Mar 2, 2016

Indeed. Working code trumps good-but-empty intentions. Onto the TODO list...

@armstrtw
Copy link
Contributor Author

@armstrtw armstrtw commented Mar 2, 2016

updated to rename those functions.

Let's have a separate discussion about how you want to change this particular code to make use of Rcpp.

eddelbuettel added a commit that referenced this pull request Mar 2, 2016
…conversion

add sanity checks for bbgDateToJulianDate
@eddelbuettel eddelbuettel merged commit 04da9c4 into master Mar 2, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@armstrtw armstrtw deleted the feature-add-checks-for-julian-date-conversion branch Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.