-
Notifications
You must be signed in to change notification settings - Fork 23
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
Schemas added for google sleep APIs #336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the schemas. They look complete, however I do have some questions and suggested some changes.
"fields": [ | ||
{ "name": "time", "type": "double", "doc": "Device timestamp in UTC (s)." }, | ||
{ "name": "timeReceived", "type": "double", "doc": "Device receiver timestamp in UTC (s)." }, | ||
{ "name": "sleepConfidence", "type": "int", "doc": "Sleep confidence value between 0 and 100." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove the sleep
prefix. I'm not sure what the confidence means. Confidence of what? In general, for probability values please use a float with range 0 - 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, here confidence means sleep confidence as mentioned here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK. Can you please add the full docstring in the link to here (except the range)? Because this provides a reason of why it is called sleepConfidence.
"namespace": "org.radarcns.passive.google", | ||
"type": "record", | ||
"name": "GoogleSleepClassifyEvent", | ||
"doc": "Sleep classification event including the sleep confidence, device motion and ambient light level.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this description, I'm not sure what is being classified, e.g. what a classify event record would tell me. Is it connected to the segment event? If so, it would be helpful to have a common identifier, e.g. segmentId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, classified events serve as supporting data for segmented events that are reported at regular intervals. In contrast, segmented events are only reported when there is sufficient confidence that the user is awake. As for the relationship between segmented and classified events, the last segmented event can also be reported whenever we subscribe to sleep data, while classified events are independently reported at fixed intervals. Despite this, is there still a need for the segmentId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Sleep Segment detection result depends on sampled device motion and ambient light readings during the past day. For the customized sleep segmentation we are using Sleep Classify events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the segmentId can be used as sleep segment data, depending on the sleep classification data from the previous day. It works in a way that segmentId will increase every time SleepSegmentEvents are recorded. Would you like this or something different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to use segmentId in that case, I didn't understand from the description what confidence meant. In the main doc, please do add something like Sleep classification event that indicates that the user is probably sleeping. Includes sleep confidence, device motion and ambient light level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
"fields": [ | ||
{ "name": "time", "type": "double", "doc": "Device timestamp in UTC (s)." }, | ||
{ "name": "timeReceived", "type": "double", "doc": "Device receiver timestamp in UTC (s)." }, | ||
{ "name": "sleepStartTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user goes to sleep." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the time
variable for startTime
instead. Then you can remove this field. In the doc of time, note that the time is when the user goes to sleep.
{ "name": "timeReceived", "type": "double", "doc": "Device receiver timestamp in UTC (s)." }, | ||
{ "name": "sleepStartTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user goes to sleep." }, | ||
{ "name": "sleepEndTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user wakes up." }, | ||
{ "name": "sleepDuration", "type": "double", "doc": "The amount of elapsed time (s), that the user was asleep." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the duration the same as endTime - time
? If so, you can remove the sleepDuration variable. If it is different, please put in the doc what the difference is and rename to durationAsleep
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK if it is the same, please remove.
{ "name": "time", "type": "double", "doc": "Device timestamp in UTC (s)." }, | ||
{ "name": "timeReceived", "type": "double", "doc": "Device receiver timestamp in UTC (s)." }, | ||
{ "name": "sleepStartTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user goes to sleep." }, | ||
{ "name": "sleepEndTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user wakes up." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to endTime
{ "name": "sleepStartTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user goes to sleep." }, | ||
{ "name": "sleepEndTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user wakes up." }, | ||
{ "name": "sleepDuration", "type": "double", "doc": "The amount of elapsed time (s), that the user was asleep." }, | ||
{ "name": "sleepStatus", "type": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to status
{ "name": "sleepEndTime", "type": "double", "doc": "The UNIX epoch time (s) for the moment when the user wakes up." }, | ||
{ "name": "sleepDuration", "type": "double", "doc": "The amount of elapsed time (s), that the user was asleep." }, | ||
{ "name": "sleepStatus", "type": { | ||
"name": "SleepStatus", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to SleepClassificationStatus
"name": "SleepStatus", | ||
"type": "enum", | ||
"doc": "The status of the sleep segment detection.", | ||
"symbols": ["STATUS_SUCCESSFUL", "STATUS_MISSING_DATA", "STATUS_NOT_DETECTED", "UNKNOWN"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename symbols to SUCCESSFUL
, MISSING_DATA
, NOT_DETECTED
and UNKNOWN
.
Hi, I have made the suggested changes and will be making the changes in the radar-commons-android as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After these changes it's really good and understandable. Thank you for your efforts!
You're welcome! Thank you for your appreciation! |
Added schemas and specifications required for google sleep api.