Skip to content

Issue1499 speed up DFA calculations#1500

Merged
vincentvanhees merged 13 commits into
mainfrom
issue1499_speedupDFA
May 1, 2026
Merged

Issue1499 speed up DFA calculations#1500
vincentvanhees merged 13 commits into
mainfrom
issue1499_speedupDFA

Conversation

@jhmigueles

@jhmigueles jhmigueles commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1499 => This PR proposes a vectorized approach to Detrended Fluctuation Analysis (DFA). The implementation replaces nested loops and repeated lm.fit calls with matrix algebra, significantly improving performance, especially for short epoch lengths, while maintaining the exact same output.

Epoch Current GGIR New proposal Speed up
60 sec 15.65 sec 3.09 sec 5x
5 sec 19.25 min 8.11 sec 142x

If finally accepted, the DFA function would not be called as the analyses are directly conducted in the SSP function, so the DFA function could be removed.

Fixes #1501 => Now, the PR also includes the calculation of alpha_1 and alpha_2 and related output estimates (SSP, ABI, SSP_short, ABI_short, SSP_long, ABI_long, and SSP_diff). The documentation has been updated to describe the new indices calculated.

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).
  • Clean code has been attempted, e.g. intuitive object names and no code redundancy.
  • Documentation updated:
    • Function documentation
    • Chapter vignettes for GitHub IO
    • Vignettes for CRAN
  • Corresponding issue tagged in PR message. If no issue exist, please create an issue and tag it.
  • Updated release notes in inst/NEWS.Rd with a user-readable summary. Please, include references to relevant issues or PR discussions.
  • If you think you made a significant contribution, add your name to the contributors lists in the DESCRIPTION, zenodo.json, and inst/CITATION files.
  • GGIR parameters were added/removed. If yes, please also complete checklist below.

If NEW GGIR parameter(s) were added then these NEW parameter(s) are:

  • documented in man/GGIR.Rd
  • included with a default in R/load_params.R
  • included with value class check in R/check_params.R
  • included in table of vignettes/GGIRParameters.Rmd with references to the GGIR parts the parameter is used in.
  • mentioned in NEWS.Rd as NEW parameter

If GGIR parameter(s) were deprecated these parameter(s) are:

  • documented as deprecated in man/GGIR.Rd
  • removed from R/load_params.R
  • removed from R/check_params.R
  • removed from table in vignettes/GGIRParameters.Rmd
  • mentioned as deprecated parameter in NEWS.Rd
  • added to the list in R/extract_params.R with deprecated parameters such that these do not produce warnings when found in old config.csv files.

@jhmigueles
jhmigueles marked this pull request as draft April 16, 2026 09:38
@jhmigueles
jhmigueles marked this pull request as ready for review April 16, 2026 11:44
@vincentvanhees

Copy link
Copy Markdown
Member

Thanks Jairo, great to see you were able to speed it up the code. Ian and I worked only with 1 minute epoch data and for us speed was acceptable at the time. So, probably that is why I did not make more effort to speed it up.

Before I do the review, could you please consider the following:

Function DFA appears to be no longer used (?) and still has the inefficient for-loop, while the new code inside function SSP is essentially what DFA does but this is not communicated. Would it not be better to keep the new functionality inside DFA such that SSP remains largely unchanged and it is clear to everyone that this PR only makes local changes to the DFA step focussed on speed and does not revise the general logic of how SSP and DFA work together?

I recognise new code snippets from SSP that came from DFA, and it is reassuring that the unit-tests are not changed, but with so many new lines it would almost seem the algorithm itself is new.

@jhmigueles

Copy link
Copy Markdown
Collaborator Author

That makes sense. As the SSP function was rather short I thought of simplifying things and doing the whole process there, but keeping the current structure is easier for review and for future references to the code. I will organize the code to keep the fluctuation calculations in DFA and the slope extraction in SSP, thanks for the suggestion!

I will push the updates shortly.

jhmigueles and others added 7 commits April 16, 2026 17:18
…e matrix with box sizes and amplitudes, not the alpha (alpha is calculated in SSP)
Merge branch 'main' into issue1499_speedupDFA

# Conflicts:
#	NEWS.md
#	vignettes/chapter13_CircadianRhythm.Rmd

@vincentvanhees vincentvanhees left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now Jairo. I am now merging this.

@vincentvanhees
vincentvanhees merged commit aa520ed into main May 1, 2026
7 checks passed
@vincentvanhees
vincentvanhees deleted the issue1499_speedupDFA branch May 1, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement piecewise DFA with alpha_1, alpha_2, and alpha_diff Performance optimisation for DFA

2 participants