Skip to content
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

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

synchon
Copy link
Member

@synchon synchon commented Aug 6, 2024

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

@LeSasse LeSasse added marker Issues or pull requests related to markers triage New issues waiting to be reviewed labels Apr 26, 2024
@fraimondo
Copy link
Contributor

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

@synchon
Copy link
Member

synchon commented Apr 29, 2024

In my opinion, we might need a custom JuniferConnectivityMeasure akin to JuniferNiftiSpheresMasker, which would allow this.

@LeSasse
Copy link
Collaborator Author

LeSasse commented Apr 29, 2024

In my opinion, we might need a custom JuniferConnectivityMeasure akin to JuniferNiftiSpheresMasker, which would allow this.

In my opinion, this is reasonable, and would also allow us to set the default to EmpiricalCovariance which is what we use mainly anyways.

@synchon
Copy link
Member

synchon commented Apr 29, 2024

In my opinion, we might need a custom JuniferConnectivityMeasure akin to JuniferNiftiSpheresMasker, which would allow this.

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?

@LeSasse
Copy link
Collaborator Author

LeSasse commented Apr 29, 2024

In my opinion, we might need a custom JuniferConnectivityMeasure akin to JuniferNiftiSpheresMasker, which would allow this.

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

@synchon
Copy link
Member

synchon commented Jul 23, 2024

In my opinion, we might need a custom JuniferConnectivityMeasure akin to JuniferNiftiSpheresMasker, which would allow this.

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

@LeSasse
Copy link
Collaborator Author

LeSasse commented Aug 6, 2024

We need to define the value for kind, i guess, so maybe something like:

kind : {"covariance", "correlation", "partial correlation", "tangent", "precision"}

can turn into:

kind : {"covariance", "spearman", "pearson",  "partial correlation", "tangent", "precision"}

What do you think

@synchon
Copy link
Member

synchon commented Aug 6, 2024

We need to define the value for kind, i guess, so maybe something like:

kind : {"covariance", "correlation", "partial correlation", "tangent", "precision"}

can turn into:

kind : {"covariance", "spearman", "pearson",  "partial correlation", "tangent", "precision"}

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: "<name> correlation", so Spearman can become: "spearman correlation".

@LeSasse
Copy link
Collaborator Author

LeSasse commented Aug 6, 2024

sounds good, then the list for implementing the spearman correlation will be:

kind : {"covariance", "spearman correlation", "correlation",  "partial correlation", "tangent", "precision"}

@synchon
Copy link
Member

synchon commented Aug 6, 2024

sounds good, then the list for implementing the spearman correlation will be:

kind : {"covariance", "spearman correlation", "correlation",  "partial correlation", "tangent", "precision"}

Exactly! :D

@synchon synchon assigned LeSasse and unassigned fraimondo and synchon Aug 6, 2024
@synchon synchon added enhancement New feature or request and removed triage New issues waiting to be reviewed labels Aug 6, 2024
@synchon synchon added this to the 0.0.6 (alpha 5) milestone Aug 6, 2024
@synchon synchon changed the title [MARKER]: [ENH]: In-built support for spearman correlation for FunctionalConnectivity* markers [ENH]: In-built support for spearman correlation for FunctionalConnectivity markers Aug 6, 2024
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.56%. Comparing base (675ccd0) to head (cc589c3).

Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
docs 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...r/external/nilearn/junifer_connectivity_measure.py 92.30% <ø> (-0.22%) ⬇️

Copy link

github-actions bot commented Aug 6, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-08-06 12:51 UTC

@synchon synchon merged commit 611bdc6 into main Aug 6, 2024
21 of 23 checks passed
@synchon synchon deleted the feat/spearman_correlation branch August 6, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request marker Issues or pull requests related to markers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants