-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH]: In-built support for spearman correlation for FunctionalConnectivity markers #335
Conversation
This also relates to #333. We need to somehow address the issue that nilearn does not accept any other "method" than these ones here: https://github.com/nilearn/nilearn/blob/4f4730163097457cf9ddb5674ffd158ee8fa822e/nilearn/connectome/connectivity_matrices.py#L497 |
In my opinion, we might need a custom |
In my opinion, this is reasonable, and would also allow us to set the default to EmpiricalCovariance which is what we use mainly anyways. |
We definitely will have more upsides than downsides from what I understand. I'll get the skeleton ready if @juaml/junifer-core agrees on this and then you can get this in while I get #333 in? |
sounds good! ping me once i can start getting active |
@LeSasse You should be able to easily implement it by tweaking https://github.com/juaml/junifer/blob/main/junifer/external/nilearn/junifer_connectivity_measure.py |
We need to define the value for kind, i guess, so maybe something like:
can turn into:
What do you think |
I'd actually keep "correlation" as is so that we're 100% compatible with nilearn but make the Spearman or any other correlation metric like so: |
sounds good, then the list for implementing the spearman correlation will be:
|
Exactly! :D |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
- Coverage 88.56% 88.56% -0.01%
==========================================
Files 115 115
Lines 5178 5175 -3
Branches 1034 1032 -2
==========================================
- Hits 4586 4583 -3
Misses 426 426
Partials 166 166
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Which marker do you want to include?
Currently it does not work out of the box, but some people like it and its a small enough thing to expect it out of the box and not really worth it have users implement their own markers for this.
Is there any publication or available code?
just rank time series and get pearson correlation
Do you have a sample code that implements this outside of junifer?
I can take a shot at implementing this if everyone agrees we should have it.
Anything else to say?
No response