Closed daehrlich closed 10 months ago
I agree with the detected bugs. Sorting should be corrected.
For the case of correct_by_target==True the number of comparisons should indeed be the number of targets (as per Novelli et al., Network Neuroscience, 2019).
For the case correct_by_target==False one should correct by: number_of_candidates = number_of_unique_combinations_of_timelags_and_source_processes.
This is because at the moment IDTxl only provides p-values per candidate, not per link as fas as I know. (But this should be double checked! Although I doubt it, there maybe per link mTE and p-values prewsent in the returned results -- if so, correction of the per-link p-values by the number of links in the network would be preferable).
Note: an efficient test at the level of candidates (sources x lags) would require to test all candidates across all targets simultaneously and enter them into the maximum statistics for inclusion. This non-hierachical testing is not supported in IDTxl at the moment and would require a major rework of the full code-path.
Merged into develop
This pull request addresses two issues with the current implementation of the statistical testing:
However, this unsorting is not correctly implemented, as the p-value argsort from before is used in a wrong way. The implementation reads
significance_array = significance_array[argsort_idxs],
where it should instead read
significance_array[argsort_idxs] = significance_array.copy()
where the copy is necessary to avoid concurrent modification.
I have implemented fixes to both issues in the branch dev_fdr_fix. Since these fixes affect the results that idtxl produces, I strongly recommend a thorough code review of the changes before a merge is performed.