incorporate greedy chi-square periodogram?

rethomics / zeitgebr

Circadian rhythm analysis and visualisation in R

11 stars 7 forks source link

incorporate greedy chi-square periodogram? #15

Open jakejh opened 4 years ago

jakejh commented 4 years ago

Hello, would you be interested in a PR for a new version of the chi-square periodogram?

https://doi.org/10.1101/2020.08.14.251512

qgeissmann commented 4 years ago

Hi @jakejh, absolutely! Also happy to discuss about how to integrate/test it if you want! thanks in advance.

jakejh commented 4 years ago

Cool, thanks. I'll get back to you.

jakejh commented 4 years ago

Hi @qgeissmann, we just realized you fixed the chi-square periodogram (https://github.com/rethomics/zeitgebr/issues/3) the same way we did! :-)

I'd suggest one of the following:

document that zeitgebr uses a "non-standard" version of CSP that has lower bias and higher accuracy and that corresponds to what we call greedy CSP
revert CSP to its standard version and add the greedy version

What do you think?

qgeissmann commented 4 years ago

Oh I see!! I remember now! yes, it seemed obvious to us that the standard CPS was broken... And I had fixed it mostly based on intuition, so I am glad we are converging on that! Would you say there is any reason to keep the standard CSP? I am more inclined to just document what we have, and then reference your work in the doc?

jakejh commented 4 years ago

That sounds great.