Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,7 @@ ENV/
.imdone

# Ruff
.ruff_cache/
.ruff_cache/

# niwrap temp data
.temp-niwrap-data
Empty file.
22 changes: 22 additions & 0 deletions neuromaps/neuromaps_nhp_utils/surface_sphere_project_unproject.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@

import os
from niwrap import workbench as wb
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More out of curiosity, why import workbench as wb?



def surface_sphere_project_unproject(sphere_in, sphere_project_to, sphere_unproject_from, sphere_out):
"""Project and unproject a surface from one sphere to another.

Parameters
----------
sphere_in : str
Path to input spherical surface.
sphere_project_to : str
Path to spherical surface to project to.
sphere_unproject_from : str
Path to spherical surface to unproject from.
sphere_out : str
Path to output spherical surface.
"""
wb.surface_sphere_project_unproject(sphere_in, sphere_project_to, sphere_unproject_from, sphere_out)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment here, but would return this call to a variable do something like:

outputs = wb.surface_sphere_project_unproject(...)

return outputs.sphere_out

return sphere_out

Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import pytest
import os
from pathlib import Path

from styxdefs import set_global_runner
from styxsingularity import SingularityRunner
from niwrap import workbench as wb
Comment on lines +5 to +7
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can all be handled now by niwrap.use_*. It will automatically set the global runner so no need to do that or import the runners

import nibabel as nib

from neuromaps.neuromaps_nhp_utils.surface_sphere_project_unproject import surface_sphere_project_unproject

my_runner = SingularityRunner(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine in this case for the scripts - I think long-term need ability to switch between different runners. Can chat more about this offline.

images={"brainlife/connectome_workbench:1.5.0-freesurfer-update": "/home/bshrestha/projects/Tfunck/neuromaps.sif"}
)
data_dir = Path("/home/bshrestha/projects/Tfunck/neuromaps-nhp/.temp-niwrap-data").resolve()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is static, you can set this directly when instantiating the runner:

my_runner = SingularityRunner(
    data_dir=...
)

my_runner.data_dir = data_dir
set_global_runner(my_runner)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment re: setting runners



@pytest.mark.parametrize(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think the parametrization is necessary here unless you plan to add additional tests. Otherwise would just have these coded in the test.

"sphere_in,sphere_project_to,sphere_unproject_from,sphere_out",
[
(
str(Path("/home/bshrestha/projects/Tfunck/neuromaps-nhp-prep/share/Outputs/Yerkes19-S1200/src-S1200_to-Yerkes19_den-32k_hemi-L_sphere.surf.gii")),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is str(Path(...)) necessary? That is, does the workbench command require a string, or will it accept a Path?

str(Path("/home/bshrestha/projects/Tfunck/neuromaps-nhp-prep/share/Inputs/Yerkes19/src-Yerkes19_den-32k_hemi-L_sphere.surf.gii")),
str(Path("/home/bshrestha/projects/Tfunck/neuromaps-nhp-prep/share/Outputs/D99-Yerkes19/src-Yerkes19_to-D99_den-32k_hemi-L_sphere.surf.gii")),
str(data_dir / "out_sphere.surf.gii"),
),
# Add more tuples here for additional test cases
]
)
def test_surface_sphere_project_unproject(sphere_in, sphere_project_to, sphere_unproject_from, sphere_out):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note for now, we may need to think about pulling in the files to do the testing here.

"""
Test surface_sphere_project_unproject wrapper function.

== Example ==
------------------------
S1200 to Yerkes19 to D99
------------------------
sphere_in = S1200_aligned_t-_Yerkes19 (Input)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an "o" in to-

project_to_sphere = Yerkes19 (Intermediate)
unproject_from_sphere = Yerkes19_to_D99 (Target)
out_sphere = str(Path(f"{data_dir}/out_sphere.surf.gii").resolve())
------------------------
"""

result = surface_sphere_project_unproject(
sphere_in, sphere_project_to, sphere_unproject_from, sphere_out
)
assert os.path.exists(result)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail because result is an object. Instead, you will want to test that result.sphere_out exists.


# check if the output file has the same number of vertices as the input file
assert nib.load(result).darrays[0].data.shape == nib.load(sphere_in).darrays[0].data.shape
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 comments here:

  1. The loading will raise an error, again because result is an object.
  2. The assertion has the possibility of failing here. With surface transformations across different spaces like this, it isn't guaranteed that the input and output transformations contain the same number of vertices. Instead, it would be better to check:
    a. The output sphere has the same shape as the target sphere
    b. The spatial locations of the vertices are near identical to the target sphere.

17 changes: 11 additions & 6 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "neuromaps"
description = "A toolbox for projecting, resampling, and comparing brain maps"
readme = "README.rst"
requires-python = ">=3.8"
requires-python = ">=3.10"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - would also note in the commit / pr comments that this change is due to <py3.10 no longer being supported (EoL)

license = {file = "LICENSE"}
keywords = ["network neuroscience", "brain maps", "annotated connectomes"]
authors = [
Expand All @@ -20,12 +20,17 @@ classifiers = [
]

dependencies = [
"numpy >=1.14.0",
"scipy >=0.17",
"matplotlib >=3.5.0",
"numpy>=1.14.0",
"scipy>=0.17",
"matplotlib>=3.5.0",
"scikit-learn",
"nibabel >=3.0.0",
"nilearn"
"nibabel>=3.0.0",
"nilearn",
"pytest>=8.3.5",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing, would create a separate dependency group with just test dependencies so that these aren't unnecessarily installed if not testing / developing.

"packaging>=25.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is packaging required?

"pandas>=2.0.3",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pandas required - if this is pinned because a minimum version is required should include a comment about it

"niwrap>=0.6.3",
"styxsingularity>=0.5.1",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this isn't needed anymore because niwrap can handle all of the the runner logic as of 0.6.x. I would look into the use_* commands from niwrap.

As a side note, aside from 1-off scripts, when building packages with niwrap, would ultimately want to be able to support all runners (e.g. docker, singulartity, etc.)

]

dynamic=["version"]
Expand Down
Loading