[ENH]: In-built support for spearman correlation for FunctionalConnectivity markers

synchon commented

2024-08-06 11:15:05 +00:00

(Migrated from github.com)

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

### 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? ```shell I can take a shot at implementing this if everyone agrees we should have it. ``` ### Anything else to say? _No response_

fraimondo commented

2024-04-29 07:20:31 +00:00

(Migrated from github.com)

This also relates to #333.

We need to somehow address the issue that nilearn does not accept any other "method" than these ones here: github.com/nilearn/nilearn@4f47301630/nilearn/connectome/connectivity_matrices.py (L497)

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 commented

2024-04-29 09:54:23 +00:00

(Migrated from github.com)

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

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

LeSasse commented

2024-04-29 10:13:31 +00:00

(Migrated from github.com)

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.

> 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 commented

2024-04-29 10:18:55 +00:00

(Migrated from github.com)

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?

> > 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?

👍 2

LeSasse commented

2024-04-29 10:34:04 +00:00

(Migrated from github.com)

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

> > > 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

👍 1

synchon commented

2024-07-23 06:18:04 +00:00

(Migrated from github.com)

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

> > > > 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 commented

2024-08-06 08:53:25 +00:00

(Migrated from github.com)

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

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 commented

2024-08-06 08:57:58 +00:00

(Migrated from github.com)

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".

> 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".

👍 1

LeSasse commented

2024-08-06 08:58:56 +00:00

(Migrated from github.com)

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

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

sounds good, then the list for implementing the spearman correlation will be: ``` kind : {"covariance", "spearman correlation", "correlation", "partial correlation", "tangent", "precision"} ```

synchon commented

2024-08-06 09:03:22 +00:00

(Migrated from github.com)

sounds good, then the list for implementing the spearman correlation will be:
kind : {"covariance", "spearman correlation", "correlation",  "partial correlation", "tangent", "precision"}

Exactly! :D

> sounds good, then the list for implementing the spearman correlation will be: > > ``` > kind : {"covariance", "spearman correlation", "correlation", "partial correlation", "tangent", "precision"} > ``` Exactly! :D

codecov[bot] commented

2024-08-06 11:24:38 +00:00

(Migrated from github.com)

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

@@            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%)`	⬇️

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/335?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 88.56%. Comparing base [(`675ccd0`)](https://app.codecov.io/gh/juaml/junifer/commit/675ccd01fb864fae4c8dd3f07fae26e95c804822?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`cc589c3`)](https://app.codecov.io/gh/juaml/junifer/commit/cc589c3ece7a6b49bd2fab6ac233740cba343a6a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml). <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/335/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://app.codecov.io/gh/juaml/junifer/pull/335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ 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](https://app.codecov.io/gh/juaml/junifer/pull/335/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/juaml/junifer/pull/335/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.00% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/juaml/junifer/pull/335?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [...r/external/nilearn/junifer\_connectivity\_measure.py](https://app.codecov.io/gh/juaml/junifer/pull/335?src=pr&el=tree&filepath=junifer%2Fexternal%2Fnilearn%2Fjunifer_connectivity_measure.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9leHRlcm5hbC9uaWxlYXJuL2p1bmlmZXJfY29ubmVjdGl2aXR5X21lYXN1cmUucHk=) | `92.30% <ø> (-0.22%)` | :arrow_down: | </details>

github-actions[bot] commented

2024-08-06 11:35:11 +00:00

(Migrated from github.com)

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

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-08-06 12:51 UTC

Rows
Columns

[ENH]: In-built support for spearman correlation for FunctionalConnectivity markers #335

Which marker do you want to include?

Is there any publication or available code?

Do you have a sample code that implements this outside of junifer?

Anything else to say?

Codecov Report