Skip to content
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 to part 6: DFA + fragmentation metrics + IV and IS (including major updates to IV and IS) #1028

Closed
wants to merge 88 commits into from

Conversation

vincentvanhees
Copy link
Member

@vincentvanhees vincentvanhees commented Jan 31, 2024

This PR adds the DFA functionality to GGIR part 6, and by that fixes #839 and fixes #955

By default GGIR will not perform DFA. Only when user provides argument part6DFA=TRUE, and include 6 in argument mode to the GGIR() function then DFA will be run.

I made a couple of minor changes/edits to the SSP/DFA/ABI code as provided by @iandanilevicz:

  • I renamed object file to data, because the object represents the data and not a file.
  • I moved documentation outside the function and clarified that GGIR does not use Roxygen by which the real documentation is now in the .Rd files. In the future I may change to Roxygen but not at this moment: GGIR is big and has used normal R package documentation for 10+ years, I do not want to rush such changes.
  • In SSP the if-else-logic did not work when the input has NA values, because then dfa_hat is set to value NA while a few lines lower we are extracting its non-existing second column. By moving the last couple of lines in the else-conditions it seems to work now.
  • class(file) == "data.frame" is no longer allowed and I have replaced it by inherits(x = data, "data.frame")
  • SSP expanded with data to vector conversion, which you had in DFA but I think it is good to also make that conversion inside SSP to standardise the data shape being used.
  • Unit test added
  • I have added your names to the contributor list in the DESCRIPTION file and acknowledged your contribution in the changelog (NEWS.md).

Additionally, I have now expanded this PR with:

  • a revision to g.IVIS.R (with help from Ian) and I embedded this in both GGIR part 2 and 6. Further, I have updated the documentation to reflect updated approach. Plan is to migrate all circadian rhythm documentation to a separate vignette in the upcoming months and critically review it in the process, but I will do that in a different PR.
  • Fragmentation metrics, same function as used in part 5 is now also applied per recording.
  • Fix to bug in save_part5_without_invalid save_ms5raw_without_invalid, fixes save_ms5raw_without_invalid = TRUE misses last window in recording #1129

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 file, if you think you made a significant contribution.

@vincentvanhees vincentvanhees marked this pull request as ready for review January 31, 2024 08:34
@iandanilevicz
Copy link
Collaborator

Hi Vincent,
I will answer each one of your minor modifications. Please let me know if you preffer that I write it in another branch.

  1. ok
  2. ok
  3. I made an external code to remove NA, and I forgot to include it internaly, sorry for that. Anyway your solution of not run if there is any NA is very safe. However, DFA is robust to NA (there is always the open issue of how many NAs are needed to break the method). Maybe it is the case to include a "na.cleaning" operation; we can discuss it on Friday.
  4. ok
  5. ok
  6. if I understood well, you added a test to check if the time series length is long enough for the DFA. This is a fair procedure, but in this case the minimum size should be 8 (as we need to compare at least 2 boxes within a minimum length equal to 4 each).

vincentvanhees and others added 4 commits February 2, 2024 14:44
Co-Authored-By: Victor Barreto Mesquita <54644227+victormesquita40@users.noreply.github.com>
Co-Authored-By: Ian Danilevicz <43249776+iandanilevicz@users.noreply.github.com>
Revise IV and IS calculation + integrate in both part2 and part6
@vincentvanhees vincentvanhees changed the title Add DFA functionality Add DFA functionality + Major update to IV and IS Feb 15, 2024
- moving LXMX from part 5 to part 6
- ignoring invalid windows in part 6 with new parameter includecrit.part6 to control this
- remove fragmentation metrics for the spt window in part 5 as not meaningful, part5 is focused on daytime behaviours
@vincentvanhees vincentvanhees mentioned this pull request May 6, 2024
22 tasks
- part 5 time series export now is flexible to work with any timewindow type (MM, OO, WW)
- part 6 now correctly handles O-1 as being the last onset in the time series.
- includecrit.part6 changed to refer to minimum valid fraction to be consistent with similar parameters (it was max invalid fraction in the non-released code).
- includenightcrit.part5 added to allow for controlling inclusion of windows in part 5 based on their amount of valid data during spt window. Default remains 0, which is to allow for spt windows without wear.
- fix incorrect usage of part 5 inclusion criteria testing
…_part5

revise inclusion criteria part 5 and 6 #1028
@vincentvanhees
Copy link
Member Author

closing this PR, because this is not going to be merged anytime soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants