Codecov Report

Merging #68 (6565dd5) into master (71c0a82) will increase coverage by 0.16%.
The diff coverage is 92.71%.

❗ Current head 6565dd5 differs from pull request most recent head 7a55e76. Consider uploading reports for the commit 7a55e76 to get more accurate results

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   90.35%   90.51%   +0.16%     
==========================================
  Files          20       22       +2     
  Lines        2342     2489     +147     
==========================================
+ Hits         2116     2253     +137     
- Misses        226      236      +10     

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 71c0a82...7a55e76. Read the comment docs.

@raphaelvallat I'd love to help here but you should know I've never reviewed a PR before. I wouldn't be asking for any direction -- I'll dig that up on my own -- but I guess I'd ask for some leeway on ignorance/mistakes and it might take longer than you're expecting. So I understand completely if you'd rather go with someone else. If you want me to go through with it let me know what kind of time frame you'd expect or at least prefer.

Hi @olmurillo and @remrama!

Thank you both for replying! To give more context, I want to start making changes in YASA as "peer-reviewed" pull requests (PR) instead of just me pushing commits to the main branch that no one will ever check. I couldn't think of anyone else to help with the review, but please feel free to just say no and/or recommend someone else!

If you have the time to help, here are some guidelines for reviewing the PR:

• Please only focus on the heart.py and hypno.py files. You can discard the other files (i.e. mark them as "Viewed").
• I think I mostly need help with making sure that the documentation of the two new functions is clear and understandable (especially since you are both native English speakers).
• As for the actual code, I hope that it is commented and written in such a way that you should be able to understand the overall pipeline. If it's not the case, please let me know. You should mostly try to a) spot blatant mistakes that I may have missed, b) see if the outputs of the function make sense or if there is something we can do that would be more useful/generalizable.
• The best way to review the PR is simply to try the new functions and make sure that they work as expected. You will need to git fork YASA on your local computer, switch to the PR branch and then install the develop version of YASA by typing python setup.py develop in the YASA folder. Let me know if you need help with this.

As for timeline, I would say within the next couple of weeks would be great but totally understand if that's not possible. There's no urgency at all.

Thanks again!
Raphael

Ok this sounds great @raphaelvallat I'd love to contribute with a PR review. I expect to be able to have this done within the "couple of weeks" timeframe. I'll let you know otherwise, or if anything comes up.

One place to improve flexibility in hypno_find_periods() (line 408 of hypno.py):
np.not_equal() does not work with strings (unless np.char.not_equal is used) and there may be cases where the hypnogram contains strings, e.g., hypno = np.array(['W', 'W', 'R', 'R']) . Currently testing, but looks like a simple switch to: loc_run_start[1:] = x[:-1] != x[1:] instead of np.not_equal(x[:-1], x[1:], out=loc_run_start[1:]) should do the trick! It should also work if hypnograms are a combination of strings and ints.

@olmurillo Thanks for catching that! I just pushed a commit implementing your changes :):
6565dd5

Thank you @remrama! Really appreciate the time and effort you put in this review :) I have just pushed a commit to implement your changes, and I have left two conversations unresolved.