Allow part 5 to export time series even if there is only one night of data. #895

vincentvanhees · 2023-09-05T14:18:53Z

In this PR:

A few small edits to ensure that part 5 can export time series even when there is only one night in the data. Fixes Export part5 time series even if there is not enough data for normal part 5 analysis #894. Updating documentation is not needed because we did not state that time series are not exported when there is only one night in the data.
When LUX is available, code adds LUX (lightpeak) column to the time series output as this may also be of interests for those working with time series output.

Checklist before merging:

Existing tests still work (check by running the test suite, e.g. from RStudio).
Added tests (if you added functionality) or fixed existing test (if you fixed a bug).
Updated or expanded the documentation.
Updated release notes in inst/NEWS.Rd with a user-readable summary. Please, include references to relevant issues or PR discussions.
Added your name to the contributors lists in the DESCRIPTION and CITATION.cff files.

…lt to POSIXct in unit test

codecov-commenter · 2023-09-05T15:52:47Z

Codecov Report

Merging #895 (67a8017) into master (a990e0f) will decrease coverage by 0.08%.
The diff coverage is 72.22%.

❗ Current head 67a8017 differs from pull request most recent head 72efd6d. Consider uploading reports for the commit 72efd6d to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
- Coverage   78.09%   78.01%   -0.08%     
==========================================
  Files          91       91              
  Lines       12118    12138      +20     
==========================================
+ Hits         9463     9469       +6     
- Misses       2655     2669      +14

Files Changed	Coverage Δ
R/g.part5.R	`81.08% <46.66%> (-0.32%)`	⬇️
R/g.part5.addfirstwake.R	`60.00% <50.00%> (-0.47%)`	⬇️
R/g.part5.savetimeseries.R	`69.23% <90.90%> (-6.21%)`	⬇️
R/g.part5.addsib.R	`75.40% <100.00%> (-6.26%)`	⬇️
R/g.report.part5.R	`85.28% <100.00%> (+0.03%)`	⬆️

jhmigueles

Everything works well. I run this in a short file and I got the timeseries. Also, I noticed that the part 5 clean report (daysummary) is generated even if it is empty (0 rows of data). It is expected to be empty as the recording is too short for part 5 analyses, but should we avoid generating this file? The full daysummary report within QC folder looks fine.

jhmigueles · 2023-09-08T09:54:12Z

R/g.part5.R

@@ -163,7 +163,7 @@ g.part5 = function(datadir = c(), metadatadir = c(), f0=c(), f1=c(),
      di = 1
      fi = 1
      SPTE_end = c() # if it is not loaded from part3 milestone data then this will be the default
-      if (length(idindex) > 0 & nrow(summarysleep) > 1) { #only attempt to load file if it was processed for sleep
+      if (length(idindex) > 0 & nrow(summarysleep) > 0) { #only attempt to load file if it was processed for sleep


Should we update the comment in this line to # 2023-09-08: to load file as long as it has 1 night available

Good idea, but adding the date is not needed because the commit itself already has a timestamp and it reduces readability to dates in the code. I am now updating the comment.

jhmigueles · 2023-09-08T09:57:10Z

R/g.part5.addsib.R

@@ -61,6 +61,7 @@ g.part5.addsib = function(ts, epochSize, part3_output, desiredtz, sibDefinition,
    redo1 = nightsi[1] - ((60/epochSize)*60) # 1 hour before first midnight
    if (redo1 < 1) redo1 = 1
    redo2 = nightsi[1] + (14*(60/epochSize)*60) # 14 hours after first midngiht
+    if (redo2 > Nts) redo2 = Nts


Not sure if this can interact with my changes in #892. I don't think so, but maybe it is safer to merge the two branches and test them together before merging them into master.

I just merged all in this PR. The exporting of the short time series file still works.

Note that this line only checks that redo2 is not beyond the end of the measurement because the line before potentially incorrectly assumes that there is always 14 hours extract after the first night. If it is then the new line clips redo2 to the number of rows in ts (Nts). So, this should not do any harm.

@jhmigueles Would you mind testing this branch again with the files you used to test #892 to make sure that functionality is not broken?

Just did it and everything works well.

vincentvanhees · 2023-09-12T12:48:11Z

@jhmigueles

Everything works well. I run this in a short file and I got the timeseries. Also, I noticed that the part 5 clean report (daysummary) is generated even if it is empty (0 rows of data). It is expected to be empty as the recording is too short for part 5 analyses, but should we avoid generating this file? The full daysummary report within QC folder looks fine.

For me no day or recordings level part5 csv reports are created, only the time series files in ms5.outraw. Could you share more details on your short recording or send me the part1 output + your script to reproduce it?

jhmigueles · 2023-09-13T05:36:55Z

For me no day or recordings level part5 csv reports are created, only the time series files in ms5.outraw. Could you share more details on your short recording or send me the part1 output + your script to reproduce it?

I just did this:

create_test_acc_csv(sf = 3, Nmin = 1440*1.5)

GGIR(datadir = "123A_testaccfile.csv",
     outputdir = "./", studyname = "testSF",
     save_ms5rawlevels = TRUE,
     save_ms5raw_format = "RData",
     save_ms5raw_without_invalid = FALSE)

And I got the file "results/part5_daysummary_MM_L40M100V400_T5A5.csv" generated, containing only the column names (not a big problem as there are no data in the file anyway).

vincentvanhees · 2023-09-13T07:45:31Z

I just did this:

Thanks, I managed to reproduce it and made a minor edit to prevent g.report.part5 from storing the csv files if there are no rows.

vincentvanhees added 3 commits September 4, 2023 17:25

Fixes #894

b870fbb

#894 add lux to part5 time series output

58c1e86

updating changelog and release date

504b837

vincentvanhees marked this pull request as ready for review September 5, 2023 14:18

vincentvanhees added 2 commits September 5, 2023 17:19

fix issue with previous commit and directly updated unnecessary POSIX…

8400e35

…lt to POSIXct in unit test

attempt to fix GitHub Actions issue for unbuntu oldrel

2bfb7cf

vincentvanhees added 2 commits September 5, 2023 18:03

Update read.myacc.csv.R

174ace8

undo changes to POSIX as it is causing new issues

8e5fb2b

vincentvanhees requested a review from jhmigueles September 5, 2023 16:13

jhmigueles reviewed Sep 8, 2023

View reviewed changes

vincentvanhees added 2 commits September 12, 2023 13:43

Merge branch 'master' into issue894_export_short_ts

0adc66d

rephrase comment to clarify that at least 1 night is needed #895

f463425

jhmigueles approved these changes Sep 13, 2023

View reviewed changes

prevent storing zero row part5_daysummary_cleaned.csv

72efd6d

vincentvanhees merged commit c7bfa8c into master Sep 13, 2023
8 checks passed

vincentvanhees deleted the issue894_export_short_ts branch September 13, 2023 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow part 5 to export time series even if there is only one night of data. #895

Allow part 5 to export time series even if there is only one night of data. #895

vincentvanhees commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading

jhmigueles left a comment

jhmigueles Sep 8, 2023

vincentvanhees Sep 12, 2023

jhmigueles Sep 8, 2023

vincentvanhees Sep 12, 2023

jhmigueles Sep 13, 2023

vincentvanhees commented Sep 12, 2023

jhmigueles commented Sep 13, 2023

vincentvanhees commented Sep 13, 2023

Allow part 5 to export time series even if there is only one night of data. #895

Allow part 5 to export time series even if there is only one night of data. #895

Conversation

vincentvanhees commented Sep 5, 2023 • edited Loading

codecov-commenter commented Sep 5, 2023 • edited Loading

Codecov Report

jhmigueles left a comment

Choose a reason for hiding this comment

jhmigueles Sep 8, 2023

Choose a reason for hiding this comment

vincentvanhees Sep 12, 2023

Choose a reason for hiding this comment

jhmigueles Sep 8, 2023

Choose a reason for hiding this comment

vincentvanhees Sep 12, 2023

Choose a reason for hiding this comment

jhmigueles Sep 13, 2023

Choose a reason for hiding this comment

vincentvanhees commented Sep 12, 2023

jhmigueles commented Sep 13, 2023

vincentvanhees commented Sep 13, 2023

vincentvanhees commented Sep 5, 2023 •

edited

Loading

codecov-commenter commented Sep 5, 2023 •

edited

Loading