wadpac / GGIRread

Functions for reading accelerometer data files
https://CRAN.R-project.org/package=GGIRread
Apache License 2.0
5 stars 3 forks source link

Iterate through cwa files using native cwa blocks instead of 300-sample pages #51

Closed l-k- closed 10 months ago

l-k- commented 10 months ago

(This might be a bad idea because I don't know the history behind the 300-sample pages that readAxivity uses -- but I thought I'd ask!) What if we got rid of the 300-sample pages, and changed the start and end parameters of readAxivity to mean the start and end cwa block, not start and end 300-sample page? This makes the code a lot simpler.

I tried this out because I thought it could make GGIR run faster on long cwa files. Unfortunately I'm seeing only a very small speed up, maybe 5-10%. But I still like how much simpler the code is, since we don't have to crawl through the file searching for a specific timestamp.

But maybe the 300-sample pages do cary some significance that I don't know about. The new approach also doesn't allow specifying timestamps for start and end, only block numbers.

l-k- commented 10 months ago

I also opened a small companion PR in the GGIR repo, to modify blocksize values, to account for the fact that GGIR "blocks" will now consist of 80-sample units, not 300-sample units: https://github.com/wadpac/GGIR/pull/886

codecov-commenter commented 10 months ago

Codecov Report

Merging #51 (4de140e) into main (171d30b) will increase coverage by 0.13%. The diff coverage is 92.85%.

:exclamation: Current head 4de140e differs from pull request most recent head c519a87. Consider uploading reports for the commit c519a87 to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   86.65%   86.79%   +0.13%     
==========================================
  Files           9        9              
  Lines         944      901      -43     
==========================================
- Hits          818      782      -36     
+ Misses        126      119       -7     
Files Changed Coverage Δ
R/readAxivity.R 85.17% <92.85%> (+0.18%) :arrow_up:
vincentvanhees commented 10 months ago

(This might be a bad idea because I don't know the history behind the 300-sample pages that readAxivity uses -- but I thought I'd ask!) What if we got rid of the 300-sample pages, and changed the start and end parameters of readAxivity to mean the start and end cwa block, not start and end 300-sample page? This makes the code a lot simpler.

The 300-sample page had no meaning in relation to Axivity and was aimed to be consistent with page size in the GENEActiv data, where a page reflects a data block with real meaning, e.g. each page there has its own header. So, these updates should not break functionality.

The new approach also doesn't allow specifying timestamps for start and end, only block numbers.

This is not a problem for GGIR. It would have been nice to keep it but given the major improvement in readability I think it is fine to remove it.

I also opened a small companion PR in the GGIR repo, to modify blocksize values, to account for the fact that GGIR "blocks" will now consist of 80-sample units, not 300-sample units: https://github.com/wadpac/GGIR/pull/886

You mean that it is either 40, 80 or 120 depending on whether AX6 or AX3 is used, and in the case of AX3 whether.