Default value of daysleep_offset missing for single-midnight files.

[l-k-]

Hello!

I ran g.shell.GGIR with all the default parameters on a sample 123A_testaccfile.csv file generated by create_test_acc_csvz(), and I got an error during part 3 processing:

Part 3

Busy processing ... see /Users/lk/acc_results/output_acc_data/ms3 for progress

Errors and warnings for 123A_testaccfile.csv.RData$message
[1] "object 'daysleep_offset' not found"

$call
g.sib.det(M, IMP, I, twd = c(-12, 12), timethreshold = timethreshold,
anglethreshold = anglethreshold, acc.metric = acc.metric,
desiredtz = desiredtz, constrain2range = constrain2range,
dayborder = dayborder, myfun = myfun)

The problem is that in g.sib.det.R, daysleep_offset variable isn't assigned a default value for single-midnight files. This was addressed for multi-midnight files in #325 , but the single-midnight scenario was missed.

I'm sending a quick one-line pull request to address this.

But I'm also wondering if you'd want g.sib.det.R rewritten to to combine the two nearly identical "Estimate Sleep Period Time window" blocks into one (I would give it a shot, but don't want to step on anyone's toes!)

I'm concerned that with time people will be inconsistently editing these two blocks and they will start diverging, creating issues. For instance I'm wondering if it's indeed correct that daysleep_offset is accounted for in

GGIR/R/g.sib.det.R

Lines 336 to 337 in d0af9d0

	sptwindow_HDCZA_end[j] = (inbedout$sptwindow_HDCZA_end/(3600/ws3)) + startTimeRecord + daysleep_offset
	sptwindow_HDCZA_start[j] = (inbedout$sptwindow_HDCZA_start/(3600/ws3)) + startTimeRecord + daysleep_offset

but not in its single-midnight equivalent:

GGIR/R/g.sib.det.R

Lines 245 to 246 in d0af9d0

	sptwindow_HDCZA_end[1] = inbedout$sptwindow_HDCZA_end/(3600/ws3) + startTimeRecord
	sptwindow_HDCZA_start[1] = inbedout$sptwindow_HDCZA_start/(3600/ws3) + startTimeRecord

I might be overthinking this a bit, but please let me know what your thoughts on this code duplication are.

thank you!

-Lena

[vincentvanhees]

Thank you Lena. Yes, I fully agree that this needs addressing. If you are happy to give it a shot then you are definitely welcome.
If anything is unclear then let us know.

I now see that the 1 night scenario is poorly covered by unit tests at the moment. So, it may be good to start creating a short unit test for your scenario.

I will review your pull request in the upcoming days, bit busy at the moment.

[l-k-]

Vincent -- sounds great, I'll start with a unit test!

[vincentvanhees]

regarding your observation ...but not in its single-midnight equivalent:...
I think you are right that it should be there too.

The situation is that I developed the original functionality for a study where first and last day of recording had to be ignored. Last year @m-patterson helped improve the code to also make it work for first and last day of recordings. So, I think we simply haven't looked at single day recordings enough yet to spot such errors.

[vincentvanhees]

I am now working in branch issue418_sibreport where I have addressed the above issue. The double code for the single-night and multi-night scenario is removed.

That branch is part of a bigger development related to handling nap and non-wear diaries, which I anticipate finishing in July. I will create a PR for the entire development and invite you @l-k- and possibly others to review.

[l-k-]

@vincentvanhees thank you for letting me know! I thought I'd find the time to refactor this code, but didn't get a chance. I will definitely be happy to participate in the review!

[vincentvanhees]

Closing this issue now, but feel free to re-open if the issue re-appears.