-
Notifications
You must be signed in to change notification settings - Fork 5
Don't store zero steps #84
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
Open
blootsvoets
wants to merge
2
commits into
dev
Choose a base branch
from
ignore-default-data
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
it still does not solve the problem where the data is present and is valid (like someone is sitting so 0 steps, which may be valuable in some use cases and should be collected) vs when someone is not wearing the device (these need to be discarded).
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 point is that for my account, Fitbit always sends 0, whether I'm wearing the device or not. It basically gives no information except that the account is being synced.
Uh oh!
There was an error while loading. Please reload this page.
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, but it would still be useful to collect steps for cases where the 0 steps is valid, for instance, you could find out if someone is at rest (when steps are 0) but there is HR data, meaning, they are wearing the device but not moving. Similarly, in some tasks (like the 6minute walk test), the subjects are instructed that they can use a chair to sit if needed in between the test, this would be 0 steps too and valuable information. So, collecting 0 steps could be useful in certain scenarios. Not collecting all steps which are 0 will remove this information too.
If this is required on the dashboard, can the query to db handle this (like
WHERE steps != 0
)?Or can be handled using KSQL so it doesn't even end up in the db.
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.
It’s just that with this method, the connector will always update to the latest step count, even if it’s zero and no steps were taken YET. But the connector then won’t go back to retrospectively evaluate whether steps were still reported by the app at a later time. I’m just saying zero steps contains NO information, it could be:
the only cases it eliminates is:
the latter case you can eliminate by looking at other topics or comparing with the Fitbit website. If so, then having a record with zero steps gives exactly the same information as having no record at all. The advantage of not storing a record then, is that the request generator will recognize that no valid steps data is available yet for a given time and it will try to re-fetch it.
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 I see, I understand the reasoning behind the request generator, I am happy with the change but I think we should still consult with the data scientists and also how backwards compatible this would be for analysis if they were already using 0 steps for something. I think 0 steps (although could be misleading) is more intuitive to understand than missing step data (which would now mean either actual missing data or any of the cases you mentioned above) and we should make sure this is documented extensively (perhaps in the schema and specs).
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 will mess with the analysis, otherwise I could see whether we can make a route-specific change regarding what qualifies as a successful call, and send the data to Kafka regardless.
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 handling the step route as a special case makes sense.