Add datetime to CSV #1009
Add datetime to CSV #1009
Conversation
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
| << curr_tm->tm_hour << ":" << std::setw(2) | ||
| << curr_tm->tm_min << ":" << std::setw(2) | ||
| << curr_tm->tm_sec; | ||
| writer(current_datetime_msg.str()); |
There was a problem hiding this comment.
This is not pretty and can be replaced by
const std::time_t current_datetime = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
std::stringstream current_datetime_msg;
current_datetime_msg << std::put_time(std::localtime(¤t_datetime), "%F %T");
writer("start_datetime = " + current_datetime_msg.str());once we up the minimum required g++ to 5+. g++ 4.9 does not have std::put_time even though its part of C++11...
I also tried using std::strftime and char arrays but that produced a few garbage characters.
|
Ready for review. |
|
Last call for reviews. |
|
Can you give me today? |
|
No one is racing to review this so take your time :) |
|
I see no tests. Do we say that comments like these are not required to be tested? What would happen downstream if the date-time is not present or messed up? |
|
And would it make sense to add the end time as well? |
|
And what about time-zone? |
Yeah, I am not sure what can we test in Cmdstan without doing some sort of parsing in Python which seems a bit overkill to add another script just for this test. There will be tests in cmdstanr (if this gets in) if that helps. We also dont tests other coments if they are genereated or not (like
This does not touch or change anything in downstream (Math/Stan services) so nothing. And CSV readers in upstream (rstan, cmdstanr/py) do not care about the number of comments or position nor should they. They only look for specific comments (if that info is needed, but absence of presece of this comment does not change that).
Hm, maybe, though that can also be obtained from start time + running time. The problem is that the way CSV is printed that would add another comment after the CSV and we are trying to avoid that as the format or even is causing problems for almost all fast CSV readers in R and Python and they need to be handheld a bit.
Hm, good call. Will add a "UTC+3", "UTC-2". |
|
What about stansummary printing out the date time... and that we test? |
|
That means touching stansummary and I really dont want to do exapand stansummary for no reason except to test. Will try to add a test to parse in C++. |
|
I fixed it to the UTC timezone and added a test. It checks that the start_datetime was found, that the year matches and that the start_datetime ends with "UTC". |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
If this PR is merged I would just proceed with the release. |
|
Looking at it now. |
|
Thanks for the quick review. |
Summary:
Adds the timestamp to the resulting CSV to close #1006
Example:
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Rok Češnovar
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: