juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

Marker/edge centric fc #169

Merged
LeSasse merged 16 commits from marker/edge_centric_fc into main 2023-01-19 13:40:02 +00:00
Conversation 22 Commits 16 Files changed 10 +386 -8
LeSasse commented 2023-01-04 13:15:29 +00:00 (Migrated from github.com)
Copy link
  • fix #64
  • description of feature/fix
  • tests added/passed
  • add an entry to the latest changes

This pull request adds an implementation for EdgeCentricFCParcels and EdgeCentricFCSpheres following the API of the functional connectivity base class.

* [x] fix #64 * [x] description of feature/fix * [x] tests added/passed * [x] add an entry to the [latest changes](../docs/changes/latest.inc) This pull request adds an implementation for EdgeCentricFCParcels and EdgeCentricFCSpheres following the API of the functional connectivity base class.
codecov[bot] commented 2023-01-04 13:16:23 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #169 (1dc04b4) into main (844fa6f) will increase coverage by 0.02%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   93.41%   93.44%   +0.02%     
==========================================
  Files          73       75       +2     
  Lines        2794     2836      +42     
  Branches      502      508       +6     
==========================================
+ Hits         2610     2650      +40     
- Misses        125      126       +1     
- Partials       59       60       +1
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 93.43% <95.55%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
junifer/markers/__init__.py 100.00% <ø> (ø)
...nnectivity/edge_functional_connectivity_spheres.py 88.88% <88.88%> (ø)
...unifer/markers/functional_connectivity/__init__.py 100.00% <100.00%> (ø)
...nnectivity/edge_functional_connectivity_parcels.py 100.00% <100.00%> (ø)
junifer/markers/utils.py 92.85% <100.00%> (+2.38%) ⬆️
# [Codecov](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#169](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (1dc04b4) into [main](https://codecov.io/gh/juaml/junifer/commit/844fa6ff0893a272701d92d1e1f3b246176a5f57?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (844fa6f) will **increase** coverage by `0.02%`. > The diff coverage is `95.55%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/169/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/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #169 +/- ## ========================================== + Coverage 93.41% 93.44% +0.02% ========================================== Files 73 75 +2 Lines 2794 2836 +42 Branches 502 508 +6 ========================================== + Hits 2610 2650 +40 - Misses 125 126 +1 - Partials 59 60 +1 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.43% <95.55%> (+0.02%)` | :arrow_up: | 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/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/markers/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL19faW5pdF9fLnB5) | `100.00% <ø> (ø)` | | | [...nnectivity/edge\_functional\_connectivity\_spheres.py](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Z1bmN0aW9uYWxfY29ubmVjdGl2aXR5L2VkZ2VfZnVuY3Rpb25hbF9jb25uZWN0aXZpdHlfc3BoZXJlcy5weQ==) | `88.88% <88.88%> (ø)` | | | [...unifer/markers/functional\_connectivity/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Z1bmN0aW9uYWxfY29ubmVjdGl2aXR5L19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [...nnectivity/edge\_functional\_connectivity\_parcels.py](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL2Z1bmN0aW9uYWxfY29ubmVjdGl2aXR5L2VkZ2VfZnVuY3Rpb25hbF9jb25uZWN0aXZpdHlfcGFyY2Vscy5weQ==) | `100.00% <100.00%> (ø)` | | | [junifer/markers/utils.py](https://codecov.io/gh/juaml/junifer/pull/169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3V0aWxzLnB5) | `92.85% <100.00%> (+2.38%)` | :arrow_up: |
synchon commented 2023-01-05 10:33:27 +00:00 (Migrated from github.com)
Copy link

Reminder to merge this PR after #107.

Reminder to merge this PR after #107.
github-actions[bot] commented 2023-01-10 10:48:24 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2023-01-19 13:44 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.2.0 :---: Preview removed because the pull request was closed. 2023-01-19 13:44 UTC <!-- Sticky Pull Request Commentpr-preview -->
fraimondo (Migrated from github.com) requested changes 2023-01-11 08:56:56 +00:00
fraimondo (Migrated from github.com) left a comment
Copy link

I think it's missing an entry in the doc/bultin section.

I think it's missing an entry in the doc/bultin section.
fraimondo (Migrated from github.com) requested changes 2023-01-11 08:59:02 +00:00
junifer/markers/functional_connectivity/edge_functional_connectivity_spheres.py
@ -0,0 +68,4 @@
) -> None:
self.coords = coords
self.radius = radius
if radius is None or radius <= 0:
fraimondo (Migrated from github.com) commented 2023-01-11 08:58:57 +00:00
Copy link

Do we really need > 0? In nilearn terms (same as junifer), radius=0 means "single voxel".

Do we really need > 0? In nilearn terms (same as junifer), radius=0 means "single voxel".
synchon (Migrated from github.com) reviewed 2023-01-11 09:45:04 +00:00
junifer/markers/functional_connectivity/edge_functional_connectivity_spheres.py
@ -0,0 +68,4 @@
) -> None:
self.coords = coords
self.radius = radius
if radius is None or radius <= 0:
synchon (Migrated from github.com) commented 2023-01-11 09:45:04 +00:00
Copy link

From nilearn's docs, radius = None extracts signal from single voxel, not sure about 0 though. I think the condition can be improved in the sense that, radius is None is valid so it can be removed. This was copied from FunctionalConnectivitySpheres, so need to update that as well. I can open a new PR to address this for both.

From nilearn's docs, `radius = None` extracts signal from single voxel, not sure about 0 though. I think the condition can be improved in the sense that, `radius is None` is valid so it can be removed. This was copied from `FunctionalConnectivitySpheres`, so need to update that as well. I can open a new PR to address this for both.
synchon commented 2023-01-11 09:45:42 +00:00 (Migrated from github.com)
Copy link

I think it's missing an entry in the doc/bultin section.

This is done now.

> I think it's missing an entry in the doc/bultin section. This is done now.
fraimondo (Migrated from github.com) reviewed 2023-01-11 12:55:44 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
fraimondo (Migrated from github.com) commented 2023-01-11 12:55:44 +00:00
Copy link

@LeSasse Is there any related publication explaining the method?

@LeSasse Is there any related publication explaining the method?
fraimondo (Migrated from github.com) reviewed 2023-01-11 12:56:59 +00:00
junifer/markers/functional_connectivity/edge_functional_connectivity_spheres.py
@ -0,0 +68,4 @@
) -> None:
self.coords = coords
self.radius = radius
if radius is None or radius <= 0:
fraimondo (Migrated from github.com) commented 2023-01-11 12:56:59 +00:00
Copy link

Yes please. The SphereAggregator takes care of that error. There's no reason to check it in other markers with a different condition (unless it invalidates the marker)

Yes please. The SphereAggregator takes care of that error. There's no reason to check it in other markers with a different condition (unless it invalidates the marker)
synchon (Migrated from github.com) reviewed 2023-01-11 12:58:01 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
synchon (Migrated from github.com) commented 2023-01-11 12:58:01 +00:00
Copy link

From the class' docstrings:

Jo et al. (2021)
Subject identification using edge-centric functional connectivity
doi: https://doi.org/10.1016/j.neuroimage.2021.118204
From the class' docstrings: ``` Jo et al. (2021) Subject identification using edge-centric functional connectivity doi: https://doi.org/10.1016/j.neuroimage.2021.118204 ```
fraimondo (Migrated from github.com) reviewed 2023-01-11 13:25:25 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
fraimondo (Migrated from github.com) commented 2023-01-11 13:25:24 +00:00
Copy link

Can you add this to the docs?

Can you add this to the docs?
synchon (Migrated from github.com) reviewed 2023-01-11 13:26:19 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
synchon (Migrated from github.com) commented 2023-01-11 13:26:19 +00:00
Copy link

Like you mean in the builtin.rst?

Like you mean in the `builtin.rst`?
LeSasse (Migrated from github.com) reviewed 2023-01-11 13:27:23 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
LeSasse (Migrated from github.com) commented 2023-01-11 13:27:23 +00:00
Copy link

we can also add this one as it was published a bit earlier (by the same group): https://www.nature.com/articles/s41593-020-00719-y but the 2021 one was the one i used as a reference for implementation

we can also add this one as it was published a bit earlier (by the same group): https://www.nature.com/articles/s41593-020-00719-y but the 2021 one was the one i used as a reference for implementation
fraimondo (Migrated from github.com) reviewed 2023-01-11 13:29:39 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
fraimondo (Migrated from github.com) commented 2023-01-11 13:29:38 +00:00
Copy link

Like you mean in the builtin.rst?

yes

The idea is that people looking at markers list can know what they are.

> Like you mean in the `builtin.rst`? yes The idea is that people looking at markers list can know what they are.
LeSasse (Migrated from github.com) reviewed 2023-01-11 13:30:17 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
LeSasse (Migrated from github.com) commented 2023-01-11 13:30:17 +00:00
Copy link

this paper is also on topic but by a different group and more of a reaction to the eFC method, but may be an interesting read for users: https://www.nature.com/articles/s41467-022-29775-7

this paper is also on topic but by a different group and more of a reaction to the eFC method, but may be an interesting read for users: https://www.nature.com/articles/s41467-022-29775-7
synchon (Migrated from github.com) reviewed 2023-01-11 13:45:06 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
synchon (Migrated from github.com) commented 2023-01-11 13:45:06 +00:00
Copy link

Like you mean in the builtin.rst?

yes

The idea is that people looking at markers list can know what they are.

Which column should it go in then?

> > Like you mean in the `builtin.rst`? > > yes > > The idea is that people looking at markers list can know what they are. Which column should it go in then?
fraimondo (Migrated from github.com) reviewed 2023-01-11 15:11:33 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
fraimondo (Migrated from github.com) commented 2023-01-11 15:11:33 +00:00
Copy link

In the description.

In the description.
synchon (Migrated from github.com) reviewed 2023-01-12 09:58:40 +00:00
docs/builtin.rst
@ -182,6 +182,16 @@ Available
- Calculate (f)ALFF and aggregate using spheres placed on coordinates
synchon (Migrated from github.com) commented 2023-01-12 09:58:40 +00:00
Copy link

I have added the reference.

I have added the reference.
fraimondo (Migrated from github.com) requested changes 2023-01-17 10:42:18 +00:00
junifer/markers/utils.py
fraimondo (Migrated from github.com) commented 2023-01-17 10:42:14 +00:00
Copy link

These lines are not covered by unit tests.

These lines are not covered by unit tests.
LeSasse (Migrated from github.com) reviewed 2023-01-18 08:23:16 +00:00
junifer/markers/utils.py
LeSasse (Migrated from github.com) commented 2023-01-18 08:23:15 +00:00
Copy link

I will add a test.

I will add a test.
fraimondo (Migrated from github.com) approved these changes 2023-01-19 13:38:47 +00:00
synchon commented 2023-01-19 13:40:17 +00:00 (Migrated from github.com)
Copy link

Cheers @LeSasse!

Cheers @LeSasse!
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!169
No description provided.