Bugs in adaptive median filter implementation (Crespo algorithm)

[juliusandretti]

Greetings, hope you're doing well.

I've been running a few tests with your implementation of the Crespo algorithm confronting it with my own implementation and I've come across a few divergences, some are bugs and cause inaccuracies. On a previous issue, I told you about a bug in the inactivity mask, the ones I'm going to discuss now are newly discovered.

1. In line 1130 of your /sleep/scoring_base.py script you use the rolling class from pandas with parameter min_periods. You should check that because min_periods may produce unexpected results when applied to series containing NaNs.
2. Starting at line 1136 you use the expanding class from pandas with parameter center=True and it produces "unexpected" results. Please refer to expanding-git-issue.
3. In line 1141 you apply an index reversion using "[::-1]". You should check that because it leads to indexing problems in subsequent pandas-related operations.

I hope this will help to improve the library.
Keep up the good work.

Julius

[ghammad]

Hi Julius,

thank you for the feedback. It will definitely help to improve the package.

In order to proceed now, may I ask you to:

• specifiy which branch/commit of the code you are referring to
• provide me some examples to reproduce the problems ( "unexpected results" or "might lead to problems" are hard to reproduce without guidance)

Or, if you have your own implementation of the Crespo algorithm, you could make a pull request and become a contributor of the package? Open-sourcing the project was done in that perspective.

Either way, thank you in advance for your help.

PS: I'll try to have all your issues resolved (#65 and #69) asap.

[juliusandretti]

Hello,

I'm referring to the latest commit on branch master. I'm attaching a script to this reply that I used to visualize the results of several rolling and expanding configurations, I think it will help you to understand what I'm talking about on points 1 and 2 and how it relates to your implementation. I've also added a minimal example demonstrating the issue with the [::-1] inversion.

Hope it helps.
Julius

operation_probe.zip

[juliusandretti]

Hello,

I've just updated my first post. The link was missing on point 2, please check it out. Also, I've noticed that I had forgotten to write about one last point, so let me add it now:

4. Slicing with [... : -1] causes the last point to be ignored. Correct slice should be [... : ]

Julius