search

ojwalch / sleep_classifiers

Classify sleep from heart rate and acceleration via Apple Watch
183 stars 84 forks source link

Variable mixup in Curve Performance Builder #3

Closed yorickvanzweeden closed 4 years ago

yorickvanzweeden commented 4 years ago

Hi Olivia,

I might be mistaken, as the build_three_class_roc_with_binary_search is a bit of a tough read, but I think there is a mistake in this code: https://github.com/ojwalch/sleep_classifiers/blob/375f50e8fc378dc5b32ca67489c7125625da3566/source/analysis/performance/curve_performance_builder.py#L261-L265

rem_roc_performance uses cumulative_nrem_accuracies, and vice versa. Should that be correct?

(I'm working on a CNN for this dataset, and I wanted to compare with your performance)

ojwalch commented 4 years ago

Thanks for catching that! Yep, that looks like a mixup. The good news is that those two vectors should be the same if the binary search worked. I’ll take a look this week and let you know if fixing that changes anything.

Best,

Olivia

On Apr 27, 2020, at 4:54 AM, Yorick van Zweeden notifications@github.com wrote:

 Hi Olivia,

I might be mistaken, as the build_three_class_roc_with_binary_search is a bit of a tough read, but I think there is a mistake in this code: https://github.com/ojwalch/sleep_classifiers/blob/375f50e8fc378dc5b32ca67489c7125625da3566/source/analysis/performance/curve_performance_builder.py#L261-L265

rem_roc_performance uses cumulative_nrem_accuracies, and vice versa. Should that be correct?

(I'm working on a CNN for this dataset, and I wanted to compare with your performance)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

ojwalch commented 4 years ago

I've updated the file with this fixed and added a note about it to the README. I've also added comments in the function to clarify what it does. Thanks again for catching this!