Skip to content

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Jul 24, 2025

  • FIX: Use class attribute instead of outer variable in PET test
  • TST: Use existing random spatial data generation fixture in PET test
  • TST: Add a fixture to create random PET data for tests

@jhlegarreta jhlegarreta requested a review from mnoergaard July 24, 2025 01:50
@jhlegarreta jhlegarreta force-pushed the ref/use-test-data-gen-pet branch 3 times, most recently from 23b4f44 to 13c2a38 Compare July 24, 2025 01:58
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jul 24, 2025

@mnoergaard a few notes here:

  • Please check that these changes make sense, and that the PET data generation fixture makes sense (e.g. default values, etc.).
  • When instantiating PET objects, my IDE says Unexpected argument for the midframe argument, but I cannot find the reason. Feel free to investigate.

Thanks.

Edit: I will rebase this on main once PR #178 gets merged.

@jhlegarreta jhlegarreta force-pushed the ref/use-test-data-gen-pet branch 3 times, most recently from c71a93b to fa28216 Compare July 24, 2025 02:24
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jul 24, 2025

Not sure why new values are required here: the seed has not changed here, and the function does not use the fixtures:
https://github.com/nipreps/nifreeze/pull/181/files#diff-0a5cb82dca1264d4dbcc31121b62678b639ee753c45919b85fcbec33611d96ddR155

Any idea @oesteban ?

@jhlegarreta jhlegarreta force-pushed the ref/use-test-data-gen-pet branch 3 times, most recently from 8cc5c06 to 72b0170 Compare July 24, 2025 03:25
pet_dataobj, affine = _generate_random_uniform_spatial_data(
request, (*vol_size, n_frames), 0.0, 1.0
)
brainmask_dataobj = rng.choice([True, False], size=vol_size).astype(np.uint8)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error

   >           raise ValueError('could not interpret dimensions')
  E           ValueError: could not interpret dimensions
  
  /home/runner/work/nifreeze/nifreeze/.tox/py312/lib/python3.12/site-packages/scipy/sparse/_base.py:862: ValueError

in
https://github.com/nipreps/nifreeze/actions/runs/16487019808/job/46613619943?pr=181#step:11:3671

seems to happen because brainmask_dataobj is of type np.uint8. I used that for the sake of consistency, but the fact it should may be a boolean has been raised previously. If that is changed to bool the test at issue passes. If one instantiates the mask in the removed _create_dataset function as a np.uint8 the same error is raised.

@jhlegarreta jhlegarreta force-pushed the ref/use-test-data-gen-pet branch 2 times, most recently from 91fad45 to 66f68ca Compare July 24, 2025 04:01
Use class attribute instead of outer variable in PET test dummy class.
Use existing random spatial data generation fixture in PET data test for
the sake of consistency.
Add a fixture to create random PET data for tests.

Adopt the fixture across PET tests.
Set new spike expected indices in test after new fixture addition

Similar to commit 4926a35.

Fixes:
```
  FAILED test/test_analysis.py::test_identify_spikes - assert False
   +  where False = <function array_equal at 0x7ffa597b79b0>(
array([ 42,  48,  61,  80,  98, 103, 113, 143, 324, 387, 422, 436, 449]),
array([ 82,  83, 160, 179, 208, 219, 229, 233, 383, 389, 402, 421, 423,\n       439, 444]))
   +    where <function array_equal at 0x7ffa597b79b0> = np.array_equal
```

raised in:
https://github.com/nipreps/nifreeze/actions/runs/16485937114/job/46610541473?pr=181#step:11:918
@jhlegarreta jhlegarreta force-pushed the ref/use-test-data-gen-pet branch from 66f68ca to 50dc751 Compare August 2, 2025 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant