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

Sum counts #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sum counts #33

wants to merge 1 commit into from

Conversation

rouzbeh
Copy link
Contributor

@rouzbeh rouzbeh commented Jul 8, 2022

This PR fixes a suspected bug. It seems that counts should be summed over each minute, and not averaged. The current counts are not integers, which doesn't seem to be correct.

I think this is an oversight, but please check!

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #33 (7e7b4e5) into main (33ec0e6) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main      #33   +/-   ##
=======================================
  Coverage   47.11%   47.11%           
=======================================
  Files          91       91           
  Lines        7749     7749           
  Branches     1481     1481           
=======================================
  Hits         3651     3651           
  Misses       4027     4027           
  Partials       71       71           
Impacted Files Coverage Δ
src/biopsykit/signals/imu/activity_counts.py 38.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33ec0e6...7e7b4e5. Read the comment docs.

@richrobe
Copy link
Member

Hi @rouzbeh, you are totally right, this formula is wrong! Additionally, the values should be summed over 1 sec instead of 1 min to match the output in the paper by Brond et al.. I am about to update the code accordingly. If it's desired to accumulate the activity counts even more (e.g., to epochs of 30s or 1min), I will add a utility function. However these changes will also affect other parts of biopsykit, especially the biopsykit.sleep.sleep_wake_detection module, which we're currently working on and which will be ready soon. I will create a new release once we're finished with the rework!

@rouzbeh
Copy link
Contributor Author

rouzbeh commented Jul 12, 2022

Hi @richrobe . That's good to know. You may also be interested in agcounts, which is a python package published by ActiGraph and replicating our counts algorithm. A paper will be published soon about this.

@richrobe
Copy link
Member

thanks for the hint, I'll check it out! I'm also happy to have a look at your paper as soon as it's published 🙂

@rouzbeh
Copy link
Contributor Author

rouzbeh commented Aug 1, 2022

@richrobe https://www.nature.com/articles/s41598-022-16003-x

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.

None yet

2 participants