DOC: clarify that min neighbors for clusters is 0 in permutation_clus…#13774
DOC: clarify that min neighbors for clusters is 0 in permutation_clus…#13774athishdresu wants to merge 10 commits intomne-tools:mainfrom
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Will also need a doc/changes/dev/13774.other.rst with the :newcontrib: role to mention the doc improvement
| return orders, n_permutations, extra | ||
|
|
||
|
|
||
| def _permutation_cluster_test( |
There was a problem hiding this comment.
This change is in a private function so end-users will not see it. It would be better to add a note to one of the public functions
There was a problem hiding this comment.
Hi @larsoner, I've moved the note to the public functions permutation_cluster_test and spatio_temporal_cluster_test. I also added the changelog entry in doc/changes/dev/13774.other.rst
Ready for another look! Thanks for the guidance
|
Hi @larsoner, I've completed the updates you suggested - I moved the note to the public functions and added the changelog entry in doc/changes/dev/13774.other.rst. I also officially submitted my GSoC proposal today! Whenever you have a moment, could you please approve the CI workflows to run so we can verify the tests? Thank you for all the guidance so far! |
| Note: By default, the minimum number of neighboring | ||
| points (e.g., channels) required to form a cluster is 0. | ||
| This means a single significant point can | ||
| technically constitute a cluster. |
There was a problem hiding this comment.
this note is in the References section. There's a "Notes" section right above.
| Note: By default, the minimum number of neighboring | ||
| points (e.g., channels) required to form a cluster is 0. | ||
| This means a single significant point can | ||
| technically constitute a cluster. |
There was a problem hiding this comment.
Don't put notes in References section.
|
@athishdresu: heads up that there's usually no need to merge main into your PR even if GitHub says "this branch is out-of-date with the base branch". It re-triggers all the CIs (some of which we pay for!) which usually we don't want/need to do. It's only necessary if the CIs are failing AND we know that the failure is unrelated to this PR AND there's a fix for it already merged into main. In that case, it's appropriate to merge in main to get CIs to pass. |
|
I've moved the note to the Notes section for both functions as requested. Thank you for catching that, @drammock! |
3 participants
Hi! I am Athish M, a GSoC 2026 applicant for Project 4. I've added a note to permutation_cluster_test to clarify that the minimum number of neighbors defaults to 0, which addresses the confusion mentioned in Issue #13008. This is my first contribution to the project