-
-
Notifications
You must be signed in to change notification settings - Fork 14
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 rolling-statistics to Projects #32
base: master
Are you sure you want to change the base?
Conversation
New pull-request branch for the rolling statistics project
Etn/opt for aws
this fixes as bug where the scheduleNames were being appended to data types that did not have that data, because the merge was happening on "time" instead of "id." In other words, if the another data type happened at the same "time" as the scheduleName, then the schedule data was appended to that row of data as well.
Etn/fix schedule name merge bug
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.
@jameno great work! lots of minor/picking things in here, which are not a reflection of the general good quality of your work. I am happy to discuss if you want.
Created: 9/15/2018 | ||
author: Jason Meno | ||
dependencies: | ||
* requires Tidepool user's data with est.localTime |
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.
I understand why you made this a requirement, but think that this could be made more general by having the user specify which time field to use. I suggest adding this feature to your TODO list, rather than adding the feature now.
|
||
TODO: | ||
|
||
-Add to summary statistics: |
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.
I think that we should talk about what we would like our template to be for these types of comments, so that all of our code looks the same, AND I think that we should create a template that we can use, and other contributors can follow. In my code, I created a bulleted list with "*" like you did in line 12. Though, I am not suggesting that we do things my way, but rather that we agree upon a way, add it to a template, and that we both follow our rules/template.
@@ -0,0 +1,699 @@ | |||
#!/usr/bin/env python3 | |||
# -*- coding: utf-8 -*- | |||
# pylint: disable=C0301 |
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.
this didn't work in the spyder editor for me. did it work for you? let's talk about about this.
-Vectorize round5minutes function | ||
-Verify basal suspends are correctly implemented | ||
""" | ||
# %% REQUIRED LIBRARIES |
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.
I believe that PEP suggests two blank lines between sections
import argparse | ||
import time | ||
|
||
# %% USER INPUTS |
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.
two blank lines
rolling_df = pd.DataFrame(index=np.arange(len(df))) | ||
rolling_df["est.localTime_rounded"] = df["est.localTime_rounded"] | ||
# Loop through rolling stats for each time prefix | ||
for i in range(0, len(rolling_prefixes)): |
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, don't be like me! 😉 you already used variable "i" above
))) | ||
|
||
# Set number of points per rolling window | ||
rolling_points = np.array(pd.Series(args.rollingWindow).map(rolling_dictionary)) |
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.
replace args.rollingWindow with rolling_prefixes
# get estimated HbA1c or Glucose Management Index (GMI) | ||
# GMI(%) = 3.31 + 0.02392 x [mean glucose in mg/dL] | ||
# https://www.jaeb.org/gmi/ | ||
rolling_df[rolling_prefixes[i]+"_cgm_eA1c"] = 3.31 + (0.02392*rolling_df[rolling_prefixes[i]+"_cgm_mean"]) |
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.
let's call this GMI
below54_dur = 5*rle_below54[0][np.where((rle_below54[2] == True) & (rle_below54[0] >= 3))] | ||
df["event-below54"] = False | ||
df.loc[below54_loc, "event-below54"] = True | ||
df["dur-below54"] = 0 |
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.
when specifying durations, I find that including the units in the variable name or colHeading can be useful. Good to know that we are talking about minutes here.
daily_df = daily_df.at_time(daytime_start) | ||
|
||
# Move time back 6 hours so that each row represents the appropriate day | ||
daily_df.index = daily_df.index-dt.timedelta(hours=6) |
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.
I would change this so that it is only giving the date, not date time. However, I would also include the start dateTime and the end dateTime to be specific about what time interval you are talking about (i.e., 2017-01-01 06:00 is the start dateTime and 2017-01-02 05:00 is the endTime for the day = 2017-01-01).
- Add README - Add .npmignore - Add command line tool
…/loop-report-parser
… of basal_rate_schedule
Rpw/loop report parser
Refactored code for rolling statistics
…olling-statistics
Remove metrics argument
Formatting
Added filename input details
New pull-request branch for the rolling statistics project