Sri brown jc #1
Sri brown jc #1
Conversation
…nvert ChronoRecords to Joda Intervals (2)Split up overflowing dates into two intervals (3) SRI Implementation using BitSets (4) Check continuous time intervals
g1isgone
force-pushed
the
sri_brown_jc
branch
from
b2a8424 to
85075ba
Compare
| * @param endTime EndTime of an interval that overflows | ||
| * @return Interval[] with spliced times | ||
| */ | ||
| private Interval[] splitOverFlowingDate(DateTime startTime, DateTime endTime){ |
There was a problem hiding this comment.
Maybe check for null inputs and that startTime and endTime are valid (startTime should be before endTime)
There was a problem hiding this comment.
Also that startTime and endTime are in fact on separate days
| * @return nonOverFlowingTimeIntervals | ||
| */ | ||
| public List<Interval> convertChronoRecordsToTimeIntervals(ChronoRecords records ){ | ||
| List<ChronoRecord> listOfRecords = records.getRecordsList(); |
There was a problem hiding this comment.
Should check for null input and that listOfRecords is also not null
There was a problem hiding this comment.
fair point we could add it in but based on other parts of the code base, they don't seem to do much checking on that part 🤔
| List<ChronoRecord> listOfRecords = records.getRecordsList(); | ||
| List<Interval> nonOverFlowingTimeIntervals = new ArrayList<Interval>(); | ||
|
|
||
| for(int i = 0; i< listOfRecords.size(); i++){ |
There was a problem hiding this comment.
spacing is a bit inconsistent
| List<Interval> nonOverFlowingTimeIntervals = new ArrayList<Interval>(); | ||
|
|
||
| for(int i = 0; i< listOfRecords.size(); i++){ | ||
| ChronoRecord cr1 = listOfRecords.get(i); |
There was a problem hiding this comment.
cr1 isn't the most informative variable name, maybe change to currentRecord?
| if(isNextDate(intervalStart, intervalEnd)){ | ||
| // Split interval to two, i1, i2 | ||
| Interval[] splitIntervals = splitOverFlowingDate(intervalStart, intervalEnd); | ||
| nonOverFlowingTimeIntervals.add(splitIntervals[0]); |
There was a problem hiding this comment.
133, 134 can be combined using addAll, but this is very readable
There was a problem hiding this comment.
oh cool - didn't know addAll existed will change 👍
| BitSet sleepState; | ||
|
|
||
| DateTime awakeIntervalStartTime = awakeInterval.getStart(); | ||
| String awakeIntervalFrom = awakeIntervalStartTime.toString("yyyy-MM-dd"); |
There was a problem hiding this comment.
Misleading variable name, seems to imply a start time, but it's actually the day of the awake period
|
|
||
| Iterator nextDayIt = keys.iterator(); | ||
| // iterate through pairs of days to calculate the SRI | ||
| if (nextDayIt.hasNext()) { |
There was a problem hiding this comment.
use of iterator makes sense for pairs of days, but would a for loop be more readable and reduce the need for typecasting to string?
There was a problem hiding this comment.
After testing, it seems that this does not handle the case when only one element is provided. Instead, it bypasses the for loop and returns nan
There was a problem hiding this comment.
i think that should be the expected case, because when only one date is provided, you can't calculate the SRI - you need at least two dates to calculate a value
There was a problem hiding this comment.
the typecast to the string is my unfortunate design of storing dates "conveniently" - maybe we should discuss in a later commit to refactor
| float sriScore = calculateSRI(prevDaySleepStates, nextDaySleepStates); | ||
| cumulativeSRI += sriScore; | ||
|
|
||
| } else{ // TODO: throw an error? |
There was a problem hiding this comment.
If the data is missing several days of data, would it make more since simply to continue calculating where the new sequence of contiguous days begins?
There was a problem hiding this comment.
good point - this is also a design choice - because SRI in theory requires a continuous set of days. If I skip an entire week you're not really comparing the day before and after; let's keep this for now and discuss what would be best
|
|
||
| Iterator nextDayIt = keys.iterator(); | ||
| // iterate through pairs of days to calculate the SRI | ||
| if (nextDayIt.hasNext()) { |
There was a problem hiding this comment.
After testing, it seems that this does not handle the case when only one element is provided. Instead, it bypasses the for loop and returns nan
| BitSet minsInconsistentSleepState = (BitSet) prevDay.clone(); | ||
| minsInconsistentSleepState.andNot(nextDay); // store inconsistent sleep state mins in prevDayTmp | ||
|
|
||
| return (1.0f - (minsInconsistentSleepState.length()/1440.f)); // 1 - totalMinsOfInconsistentSleepState |
There was a problem hiding this comment.
Changed this to use cardinality because length is technically always 1440
| */ | ||
| public float calculateSRI(BitSet prevDay, BitSet nextDay){ | ||
| BitSet minsInconsistentSleepState = (BitSet) prevDay.clone(); | ||
| minsInconsistentSleepState.andNot(nextDay); // store inconsistent sleep state mins in prevDayTmp |
There was a problem hiding this comment.
using andNot did not yield the correct values for SRI
… Debugged calculateSRI
neil2001
force-pushed
the
sri_brown_jc
branch
from
1c7cbf1 to
518c0bd
Compare
| BitSet day1Interval = new BitSet(1440); | ||
| BitSet day2Interval = new BitSet(); | ||
| day1Interval.set(0,720); | ||
| Assert.assertEquals(1.0, stats.calculateSRI(day1Interval, day2Interval), .0001); |
There was a problem hiding this comment.
what's the 0.0001 for?
hmm .. maybe we should change the expected behavior - if one of them is empty we should probably just throw an error
There was a problem hiding this comment.
I believe you need to set a threshold for how similar two floats are when using assertEquals on floats
| day1Interval.set(0,720); | ||
| Assert.assertEquals(1.0, stats.calculateSRI(day1Interval, day2Interval), .0001); | ||
|
|
||
| // awake 24 hours |
There was a problem hiding this comment.
is this testing the scenario where you're awake for 24 hours on one day but not the other? I think you should specify that in the comment
| BitSet interval5 = new BitSet(1440); | ||
| BitSet interval6 = new BitSet(1440); | ||
| Assert.assertEquals(1.0, stats.calculateSRI(interval5, interval6), .0001); | ||
| } |
There was a problem hiding this comment.
i think we should also add the case where you're awake 24 hours the day before and 24 hours the day after as well
| BitSet day2Interval = new BitSet(1440); | ||
| day1Interval.set(0,720); | ||
| day2Interval.set(0,720); | ||
| // testing for when the intevals are the same |
There was a problem hiding this comment.
maybe add : testing for same intervals in day 1 and day 2 to emphasize pairwise date comparisons
|
|
||
| @Test | ||
| public void testCalculateSRI() { | ||
| BitSet day1Interval = new BitSet(1440); |
There was a problem hiding this comment.
i would keep all the intervals up top, so we don't have to look for intervals mid code line
| BitSet interval5 = new BitSet(1440); | ||
| BitSet interval6 = new BitSet(1440); | ||
| interval5.set(0, 720); | ||
| interval6.set(360, 1440); |
There was a problem hiding this comment.
what's the difference between this and the "single overlap" test case below?
There was a problem hiding this comment.
I think the idea was to make sure the case when only one minute overlapped was being caught as well
There was a problem hiding this comment.
ah okay maybe just comment it as you have written it ^
|
|
||
| // pretty regular | ||
| BitSet interval9 = new BitSet(1440); | ||
| BitSet interval10 = new BitSet(1440); |
There was a problem hiding this comment.
i think what we should do is instead of doing -200 store it in a variable
we should be able to deduce how the value 200 came from and from what variables
| } | ||
|
|
||
| @Test | ||
| public void testSkipDays() { |
There was a problem hiding this comment.
I thought we threw an error / print statement if we skip days - did we not?
There was a problem hiding this comment.
Ah that's right. It currently just prints, should we have it throw an error instead/as well?
… (2) Test case clean up and refactoring (3) gitignore file
…up is a consecutive list of days (2) Initial MSRI Implementation - might change depending on final metric, but does the basic cumulative differences of SRI (3) Test cases for MSRI
… separate Utils file (2) Make a separate test case for sleep regularity index (3) Revert SocialJetlagStatsTest to original test file
No description provided.