implementing surface transform and related tests#4
Conversation
kaitj
left a comment
kaitj
left a comment
There was a problem hiding this comment.
Thanks @birajstha - think this is a good first pass at it. Left a few notes (don't necessarily need action right now), a few comments / suggestions, and also had a few questions.
Feel free to either implement the changes here or open a new PR on the skeleton repo with responses here (just make sure to close this if you do this 😅)
| "nilearn", | ||
| "pytest>=8.3.5", | ||
| "packaging>=25.0", | ||
| "pandas>=2.0.3", |
There was a problem hiding this comment.
Is pandas required - if this is pinned because a minimum version is required should include a comment about it
| "packaging>=25.0", | ||
| "pandas>=2.0.3", | ||
| "niwrap>=0.6.3", | ||
| "styxsingularity>=0.5.1", |
There was a problem hiding this comment.
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.)
| "nibabel>=3.0.0", | ||
| "nilearn", | ||
| "pytest>=8.3.5", | ||
| "packaging>=25.0", |
| "nilearn" | ||
| "nibabel>=3.0.0", | ||
| "nilearn", | ||
| "pytest>=8.3.5", |
There was a problem hiding this comment.
For testing, would create a separate dependency group with just test dependencies so that these aren't unnecessarily installed if not testing / developing.
| description = "A toolbox for projecting, resampling, and comparing brain maps" | ||
| readme = "README.rst" | ||
| requires-python = ">=3.8" | ||
| requires-python = ">=3.10" |
There was a problem hiding this comment.
👍 - would also note in the commit / pr comments that this change is due to <py3.10 no longer being supported (EoL)
| # Add more tuples here for additional test cases | ||
| ] | ||
| ) | ||
| def test_surface_sphere_project_unproject(sphere_in, sphere_project_to, sphere_unproject_from, sphere_out): |
There was a problem hiding this comment.
Just a note for now, we may need to think about pulling in the files to do the testing here.
| set_global_runner(my_runner) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Don't think the parametrization is necessary here unless you plan to add additional tests. Otherwise would just have these coded in the test.
| result = surface_sphere_project_unproject( | ||
| sphere_in, sphere_project_to, sphere_unproject_from, sphere_out | ||
| ) | ||
| assert os.path.exists(result) |
There was a problem hiding this comment.
This will fail because result is an object. Instead, you will want to test that result.sphere_out exists.
| ------------------------ | ||
| S1200 to Yerkes19 to D99 | ||
| ------------------------ | ||
| sphere_in = S1200_aligned_t-_Yerkes19 (Input) |
| assert os.path.exists(result) | ||
|
|
||
| # 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 |
There was a problem hiding this comment.
2 comments here:
- The loading will raise an error, again because
resultis an object. - 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.
2 participants
Related to this issue