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

Rewrite segmentutil to be simpler and more correct #117

Merged
merged 4 commits into from
May 12, 2014

Conversation

cheddar
Copy link
Contributor

@cheddar cheddar commented May 8, 2014

Also include some timing code that I found useful when debugging and probably doesn't hurt to have included.

@jebeck Can you give this a once over? For segmentutil.js it might be easier to just look at the file, 'cause even though github is trying to show you a diff, it's a complete rewrite with nothing but the top couple of lines maintained.

Also include some timing code that I found useful when debugging and probably doesn't hurt to have included.
@jebeck
Copy link
Contributor

jebeck commented May 9, 2014

In addition to our discussion of the lack of value on some temp basals, see the update here as well, @cheddar: tidepool-org/hub#87

@cheddar
Copy link
Contributor Author

cheddar commented May 9, 2014

Ok, I looked into the lack of value and it turns out that it's happening because of the time when there were two pumps operating at the same time. We throw away the scheduled basals because that's what we decided to do when we get overlapping scheduled basals, and then there's nothing for the temp to apply to, so no percentage to multiply.

So, I'm thinking the right thing to do here is to throw away the temp as well if it has a percent. The other thing we can do is zero them out. Let me know if you prefer zeroing, 'cause I'm gonna start on implementing the throw away logic.

If we get overlapping scheduled basals, we throw them away.  This means that there are sometimes areas where we have percentage temps that don't have a scheduled basal to apply to.  We need to throw these away as well, because we don't know what the actual amount delivered was.
@cheddar
Copy link
Contributor Author

cheddar commented May 9, 2014

Errr, the changes were simple, so they are done already. @jebeck, it seems to be rendering well. Please check and merge at will.

@jebeck
Copy link
Contributor

jebeck commented May 9, 2014

@cheddar The validator is still finding segments without a value property for 530G data. Is that expected?

Had to move smooshing to a later stage, but not overlaps are handled better.  Also included a bunch of new tests to verify the behavior
@cheddar
Copy link
Contributor Author

cheddar commented May 10, 2014

@jebeck Ok, went in and fixed a bunch of stuff. This was a fun one.

I found one thing. Fixed it, uncovered another, fixed it, uncovered another, fixed it. Probably went through like 4 different bugs in both the carelink parser and the blip-side processing as well. But, I believe it is now, stupid fingers, actually complete. Blip changes are in master and I'm going to push the newest jellyfish to devel so that you can see stuffs.

jebeck added a commit that referenced this pull request May 12, 2014
…fication

Rewrite segmentutil to be simpler and more correct
@jebeck jebeck merged commit 0fb7826 into master May 12, 2014
@jebeck jebeck deleted the cheddar/segmentutil-betterification branch May 12, 2014 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants