[MARKER]: ReHo #36

2022-11-24T11:07:17Z

synchon commented

2022-11-24 11:07:17 +00:00

(Migrated from github.com)

Which marker do you want to include?

ReHo

Inputs:
List of 3D MNI coordinates and sphere size OR a mask file
A GM brain mask

Output:
An array with one value per coordinate/voxel

Notes:
With coordinates and sphere size, there are two ways to calculate this:

use all the voxels in the sphere and calculate Kendall's W
calculate for each voxel in the sphere using the neighboring voxels

The resulting values are then averaged to get a single value for each coordinate.

Is there any publication or available code?

https://www.ncbi.nlm.nih.gov/pmc/articles/PMC5021216/

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

https://pypi.org/project/kendall-w/
https://stackoverflow.com/questions/48893689/kendalls-coefficient-of-concordance-w-in-python

Anything else to say?

No response

### Which marker do you want to include? ReHo Inputs: List of 3D MNI coordinates and sphere size OR a mask file A GM brain mask Output: An array with one value per coordinate/voxel Notes: With coordinates and sphere size, there are two ways to calculate this: - use all the voxels in the sphere and calculate Kendall's W - calculate for each voxel in the sphere using the neighboring voxels The resulting values are then averaged to get a single value for each coordinate. ### Is there any publication or available code? https://www.ncbi.nlm.nih.gov/pmc/articles/PMC5021216/ ### Do you have a sample code that implements this outside of junifer? ```shell https://pypi.org/project/kendall-w/ https://stackoverflow.com/questions/48893689/kendalls-coefficient-of-concordance-w-in-python ``` ### Anything else to say? _No response_

LeSasse commented

2022-10-07 09:57:09 +00:00

(Migrated from github.com)

There is some code here to compute_reho that may or may not be useful for this marker: https://github.com/FCP-INDI/C-PAC/blob/main/CPAC/reho/utils.py#L70

LeSasse commented

2022-10-11 09:17:34 +00:00

(Migrated from github.com)

@fraimondo @synchon @kaurao @omidvarnia I did some research on previous implementations of reho and found that afni has an implementation for this in C code. Afni is also used for reho calculation in xcpengine and it is supposed to be quite efficient which is a plus, as voxelwise reho calculation can be quite time consuming. There is an interface for this using Nipype also, so using this in python/junifer should be reasonably straightforward. The only problem I can see, is that this will lead to a dependency that is likely not "straightforward" to install, and puts the burden of installation on the user. So I guess, the question is, do we want to use this, or do you think it is worth it implementing this ourselves? I have also got the afni reho code in C if anyone would like to see it I can send it.

@fraimondo @synchon @kaurao @omidvarnia I did some research on previous implementations of reho and found that [afni](https://afni.nimh.nih.gov/pub/dist/doc/program_help/3dReHo.html) has an implementation for this in C code. Afni is also used for reho calculation in [xcpengine](https://github.com/PennLINC/xcpEngine/blob/master/modules/reho/reho.mod) and it is supposed to be quite efficient which is a plus, as voxelwise reho calculation can be quite time consuming. There is an interface for this using [Nipype](https://nipype.readthedocs.io/en/latest/api/generated/nipype.interfaces.afni.utils.html#reho) also, so using this in python/junifer should be reasonably straightforward. The only problem I can see, is that this will lead to a dependency that is likely not "straightforward" to install, and puts the burden of installation on the user. So I guess, the question is, do we want to use this, or do you think it is worth it implementing this ourselves? I have also got the afni reho code in C if anyone would like to see it I can send it.

LeSasse commented

2022-10-11 14:40:25 +00:00

(Migrated from github.com)

Test bot

fraimondo commented

2022-10-11 14:59:15 +00:00

(Migrated from github.com)

I dealt with this in the past.

Option 1: python implementation but maybe inefficient
Option 2: complicated dependency
Option 3: pack a C-based implementation in our toolbox

Best solution I found so far:
Do 1 and 3. If the user does not manage to compile the C-based implementation / install dependency, it will resort to the inefficient way.

I achieved this by adding a parameter to the marker method with two possible values: c or python.

I dealt with this in the past. Option 1: python implementation but maybe inefficient Option 2: complicated dependency Option 3: pack a C-based implementation in our toolbox Best solution I found so far: Do 1 and 3. If the user does not manage to compile the C-based implementation / install dependency, it will resort to the inefficient way. I achieved this by adding a parameter to the marker `method` with two possible values: `c` or `python`.

LeSasse commented

2022-10-12 09:17:17 +00:00

(Migrated from github.com)

I think in that case it makes sense to prioritise a python implementation first, and then think of a C-based implementation afterwards if/when performance becomes an issue.

LeSasse commented

2022-10-12 10:48:21 +00:00

(Migrated from github.com)

@kaurao when you write as input 'List of 3D MNI coordinates and sphere size OR a mask file', by mask file, do you mean a binary mask file so that for each voxel with a 1 you get a reho value, or a parcellation mask, so that for every ROI you get a value?

fraimondo commented

2022-10-12 11:20:17 +00:00

(Migrated from github.com)

Indeed this are 2 ReHo markers, the parcel based and the coord based

synchon commented

2022-11-15 07:42:06 +00:00

(Migrated from github.com)

Didn't mean to unassign anyone, just assign myself. Weird GitHub UI.

github-actions[bot] commented

2022-11-24 11:11:37 +00:00

(Migrated from github.com)

PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2022-12-19 20:50 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.2.0 :---: Preview removed because the pull request was closed. 2022-12-19 20:50 UTC

codecov[bot] commented

2022-11-24 11:23:21 +00:00

(Migrated from github.com)

Codecov Report

Merging #36 (bfd8043) into main (36bfc4c) will decrease coverage by 1.29%.
The diff coverage is 76.57%.

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   94.82%   93.53%   -1.30%     
==========================================
  Files          61       66       +5     
  Lines        2395     2613     +218     
  Branches      458      483      +25     
==========================================
+ Hits         2271     2444     +173     
- Misses         83      116      +33     
- Partials       41       53      +12

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`93.52% <76.57%> (-1.29%)`	⬇️
mock	`?`

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

Impacted Files	Coverage Δ
junifer/api/cli.py	`75.60% <ø> (ø)`
junifer/markers/utils.py	`90.47% <ø> (+28.57%)`	⬆️
junifer/markers/reho/reho_estimator.py	`68.70% <68.70%> (ø)`
junifer/markers/reho/reho_base.py	`90.00% <90.00%> (ø)`
junifer/markers/reho/reho_parcels.py	`92.00% <92.00%> (ø)`
junifer/markers/reho/reho_spheres.py	`92.30% <92.30%> (ø)`
junifer/markers/__init__.py	`100.00% <100.00%> (ø)`
junifer/markers/reho/__init__.py	`100.00% <100.00%> (ø)`
junifer/storage/sqlite.py	`94.53% <100.00%> (ø)`
junifer/__init__.py	`100.00% <0.00%> (ø)`
... and 2 more

# [Codecov](https://codecov.io/gh/juaml/junifer/pull/36?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#36](https://codecov.io/gh/juaml/junifer/pull/36?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (bfd8043) into [main](https://codecov.io/gh/juaml/junifer/commit/36bfc4ce65aa50f59cc5cfac02f2b7febacc5eba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (36bfc4c) will **decrease** coverage by `1.29%`. > The diff coverage is `76.57%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/36/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://codecov.io/gh/juaml/junifer/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #36 +/- ## ========================================== - Coverage 94.82% 93.53% -1.30% ========================================== Files 61 66 +5 Lines 2395 2613 +218 Branches 458 483 +25 ========================================== + Hits 2271 2444 +173 - Misses 83 116 +33 - Partials 41 53 +12 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.52% <76.57%> (-1.29%)` | :arrow_down: | | mock | `?` | | 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. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/cli.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvY2xpLnB5) | `75.60% <ø> (ø)` | | | [junifer/markers/utils.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3V0aWxzLnB5) | `90.47% <ø> (+28.57%)` | :arrow_up: | | [junifer/markers/reho/reho\_estimator.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19lc3RpbWF0b3IucHk=) | `68.70% <68.70%> (ø)` | | | [junifer/markers/reho/reho\_base.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19iYXNlLnB5) | `90.00% <90.00%> (ø)` | | | [junifer/markers/reho/reho\_parcels.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19wYXJjZWxzLnB5) | `92.00% <92.00%> (ø)` | | | [junifer/markers/reho/reho\_spheres.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vcmVob19zcGhlcmVzLnB5) | `92.30% <92.30%> (ø)` | | | [junifer/markers/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [junifer/markers/reho/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3JlaG8vX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [junifer/storage/sqlite.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9zdG9yYWdlL3NxbGl0ZS5weQ==) | `94.53% <100.00%> (ø)` | | | [junifer/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9fX2luaXRfXy5weQ==) | `100.00% <0.00%> (ø)` | | | ... and [2 more](https://codecov.io/gh/juaml/junifer/pull/36/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | |

synchon commented

2022-11-24 12:11:15 +00:00

(Migrated from github.com)

I need to add comparison with afni 3dReHo's output but it's another rabbit hole and from what I understand, the implementation follows the publication.

I'll add the comparisons when I have the data ready.

I need to add comparison with afni 3dReHo's output but it's another rabbit hole and from what I understand, the implementation follows the publication. I'll add the comparisons when I have the data ready.

👀 1

fraimondo (Migrated from github.com) requested changes

2022-11-24 14:00:03 +00:00

junifer/markers/reho_parcels.py

output["data"] = aggregated_values["data"][np.newaxis, :]

I would change aggregated_values to output and modify in place. Less memory.

I would change `aggregated_values` to `output` and modify in place. Less memory.

junifer/markers/reho_spheres.py

same comments as before.

junifer/markers/tests/test_reho_parcels.py

How difficult is to do this before merging the PR?

synchon (Migrated from github.com) reviewed

2022-11-24 14:02:03 +00:00

junifer/markers/reho_parcels.py

output["data"] = aggregated_values["data"][np.newaxis, :]

It's the same thing: https://numpy.org/doc/stable/reference/generated/numpy.expand_dims.html
I can change if you like the other one.

> output["data"] = aggregated_values["data"][np.newaxis, :] It's the same thing: https://numpy.org/doc/stable/reference/generated/numpy.expand_dims.html I can change if you like the other one.

synchon (Migrated from github.com) reviewed

2022-11-24 14:02:38 +00:00

junifer/markers/reho_parcels.py

I would change aggregated_values to output and modify in place. Less memory.

I was trying to not modify in-place but not a bad choice either way, will change.

> I would change `aggregated_values` to `output` and modify in place. Less memory. I was trying to not modify in-place but not a bad choice either way, will change.

synchon (Migrated from github.com) reviewed

2022-11-24 14:04:09 +00:00

junifer/markers/tests/test_reho_parcels.py

I'm working on it but there's some issue with type conversions in afni which is blocking it. And, not sure if other errors will pop up after that.

fraimondo (Migrated from github.com) requested changes

2022-12-16 12:36:46 +00:00

fraimondo (Migrated from github.com) left a comment

There is also a .nii file there without a name. I think it should not be there.

There is also a `.nii` file there without a name. I think it should not be there.

junifer/markers/reho/reho_estimator.py

					
				@ -0,0 +1,510 @@

				"""Provide estimator class for regional homogeneity (ReHo)."""

This can be written only once (no need for 125 neigh separately)

Just set:

st = 1
end = 2
if nneigh == 125:
   st = 2
   end = 3

end then index as i - st : i + end, with the product as range(st, n_x - (end - 1)

This can be written only once (no need for 125 neigh separately) Just set: ``` st = 1 end = 2 if nneigh == 125: st = 2 end = 3 ``` end then index as `i - st : i + end`, with the product as `range(st, n_x - (end - 1)`

junifer/markers/reho/reho_parcels.py

					
				@ -0,0 +26,4 @@

				    use_afni : bool, optional

				        Whether to use AFNI for computing. If None, will use AFNI only

				        if available (default None).

				    reho_params : dict, optional

This needs to be documented.

What are the valid reho params?

This needs to be documented. What are the valid reho params?

junifer/markers/reho/reho_spheres.py

					
				@ -0,0 +31,4 @@

				    use_afni : bool, optional

				        Whether to use AFNI for computing. If None, will use AFNI only

				        if available (default None).

				    reho_params : dict, optional

Same as with parcels

junifer/markers/reho/tests/test_reho_map_comparison.py

The comparison might not be correct, but we can still test the implementation separately:

Things to check:

shape
values should be normalized

The comparison might not be correct, but we can still test the implementation separately: Things to check: * shape * values should be normalized

junifer/markers/reho/tests/test_reho_parcels.py

					
				@ -0,0 +34,4 @@

				        # Get BOLD output

				        reho_parcels_output_bold = reho_parcels_output["BOLD"]

				        # Assert BOLD output keys

				        assert "data" in reho_parcels_output_bold

Things to check:

shape
values should be normalized

Things to check: * shape * values should be normalized

synchon (Migrated from github.com) reviewed

2022-12-16 13:39:28 +00:00

junifer/markers/reho/tests/test_reho_map_comparison.py

I think it's better to leave this module as it is and try to get it tested by having afni in the CI as we had planned.

synchon commented

2022-12-16 13:48:23 +00:00

(Migrated from github.com)

@fraimondo I believe I have addressed your comments.

fraimondo (Migrated from github.com) requested changes

2022-12-16 14:19:56 +00:00

fraimondo (Migrated from github.com) left a comment

There are 75 Mb of NII files. We don't need those ones.

Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing.

I do that for ALFF

There are 75 Mb of NII files. We don't need those ones. Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing. I do that for ALFF

synchon commented

2022-12-16 15:21:41 +00:00

(Migrated from github.com)

There are 75 Mb of NII files. We don't need those ones.

Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing.

I do that for ALFF

I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR.

> There are 75 Mb of NII files. We don't need those ones. > > Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing. > > I do that for ALFF I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR.

fraimondo commented

2022-12-16 17:46:15 +00:00

(Migrated from github.com)

There are 75 Mb of NII files. We don't need those ones.
Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing.
I do that for ALFF

I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR.

That's exactly what I meant! Perfect!

> > There are 75 Mb of NII files. We don't need those ones. > > Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing. > > I do that for ALFF > > I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR. That's exactly what I meant! Perfect!

synchon commented

2022-12-19 09:44:39 +00:00

(Migrated from github.com)

There are 75 Mb of NII files. We don't need those ones.
Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing.
I do that for ALFF

I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR.

That's exactly what I meant! Perfect!

I have removed the .nii files and adapted the tests.

> > > There are 75 Mb of NII files. We don't need those ones. > > > Indeed I was wondering if we can just test afni v python only when we have the afni toolbox available and avoid shipping this binaries just for testing. > > > I do that for ALFF > > > > > > I'm working on getting afni into CI, testing some stuff. I can remove the .nii files and get this in if you like. I'll anyway add the gh-actions stuff in separate PR. > > That's exactly what I meant! Perfect! I have removed the `.nii` files and adapted the tests.

fraimondo (Migrated from github.com) reviewed

2022-12-19 10:31:51 +00:00

fraimondo (Migrated from github.com) left a comment

No description provided.

Lets merge once the CI is ready to test AFNI stuff

synchon commented

2022-12-19 12:10:16 +00:00

(Migrated from github.com)

Waiting for #153 now.