From 33fc4c74d74292b57821e7c4d06a804ea14d2edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Sat, 21 Dec 2024 19:55:08 -0500 Subject: [PATCH 1/5] ENH: Add `None` to b-vals iterators `size` parameter annotations Add `None` to b-vals iterators `size` parameter annotations. Edit the parameter types accordingly in the docstrings and describe what happens when `None` is provided. Fixes: ``` src/nifreeze/utils.py:30: error: Incompatible default for argument "size" (default has type "None", argument has type "int") [assignment] src/nifreeze/utils.py:30: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True src/nifreeze/utils.py:30: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase src/nifreeze/utils.py:59: error: Incompatible default for argument "size" (default has type "None", argument has type "int") [assignment] src/nifreeze/utils.py:59: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True src/nifreeze/utils.py:59: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase src/nifreeze/utils.py:108: error: Incompatible default for argument "size" (default has type "None", argument has type "int") [assignment] src/nifreeze/utils.py:108: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True src/nifreeze/utils.py:108: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase src/nifreeze/utils.py:135: error: Incompatible default for argument "size" (default has type "None", argument has type "int") [assignment] src/nifreeze/utils.py:135: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True src/nifreeze/utils.py:135: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:32 --- src/nifreeze/utils/iterators.py | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/nifreeze/utils/iterators.py b/src/nifreeze/utils/iterators.py index 0cf52d21..5e27d221 100644 --- a/src/nifreeze/utils/iterators.py +++ b/src/nifreeze/utils/iterators.py @@ -33,9 +33,10 @@ def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` - Number of volumes in the dataset - (for instance, the number of orientations in a DWI) + size : :obj:`int` or None, optional + Number of volumes in the dataset (for instance, the number of + orientations in a DWI). If `None`, the size is inferred from the `bvals` + argument in `kwargs`, if present. Returns ------- @@ -60,16 +61,17 @@ def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: """ Traverse the dataset volumes randomly. + If the `seed` key is present in the keyword arguments, initializes the seed + of Python's `random` pseudo-random number generator library with the given + value. Specifically, if `False`, `None` is used as the seed; it `True`, a + default seed value is used. + Parameters ---------- - size : :obj:`int` - Number of volumes in the dataset - (for instance, the number of orientations in a DWI) - seed : :obj:`int` or :obj:`bool` or :obj:`bool` or ``None`` - If :obj:`int` or :obj:`str` or ``None``, initializes the seed of Python's random generator - with the given value. - If ``False``, the random generator is passed ``None``. - If ``True``, a default seed value is set. + size : :obj:`int` or None, optional + Number of volumes in the dataset (for instance, the number of + orientations in a DWI). If `None`, the size is inferred from the `bvals` + argument in `kwargs`, if present. Returns ------- @@ -138,9 +140,10 @@ def centralsym_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` - Number of volumes in the dataset - (for instance, the number of orientations in a DWI) + size : :obj:`int` or None, optional + Number of volumes in the dataset (for instance, the number of + orientations in a DWI). If `None`, the size is inferred from the `bvals` + argument in `kwargs`, if present. Returns ------- From 8e7aba99cd8d79b28a327f0167b5463e084e94fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20Haitz=20Legarreta=20Gorro=C3=B1o?= Date: Sat, 21 Dec 2024 20:18:18 -0500 Subject: [PATCH 2/5] ENH: Remove unused parameter form `bvalue_iterator` prototype Remove unused parameter from method `bvalue_iterator` prototype and docstring. Use the `*_` to denote unused arguments. --- src/nifreeze/utils/iterators.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/nifreeze/utils/iterators.py b/src/nifreeze/utils/iterators.py index 5e27d221..cd0ec0c0 100644 --- a/src/nifreeze/utils/iterators.py +++ b/src/nifreeze/utils/iterators.py @@ -107,15 +107,10 @@ def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: return (x for x in index_order) -def bvalue_iterator(size: int | None = None, **kwargs) -> Iterator[int]: +def bvalue_iterator(*_, **kwargs) -> Iterator[int]: """ Traverse the volumes in a DWI dataset by growing b-value. - Parameters - ---------- - bvalues : :obj:`list` - List of b-values corresponding to all orientations of the dataset. - Returns ------- :obj:`~typing.Iterator` From 6398249653a2adfb06481cc704b183b41b7c470a Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 23 Jan 2025 09:25:33 +0100 Subject: [PATCH 3/5] enh: code suggestions from @oesteban --- src/nifreeze/utils/iterators.py | 72 ++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 28 deletions(-) diff --git a/src/nifreeze/utils/iterators.py b/src/nifreeze/utils/iterators.py index cd0ec0c0..43a2bf12 100644 --- a/src/nifreeze/utils/iterators.py +++ b/src/nifreeze/utils/iterators.py @@ -33,14 +33,16 @@ def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` or None, optional - Number of volumes in the dataset (for instance, the number of - orientations in a DWI). If `None`, the size is inferred from the `bvals` - argument in `kwargs`, if present. - - Returns - ------- - :obj:`~typing.Iterator` + size : :obj:`int` or ``None`` (optional) + Number of volumes in the dataset. + bvalues : :obj:`list` + List of b-values corresponding to all orientations of a DWI dataset. + If ``size`` is provided, this argument will be ignored. + Otherwise, ``size`` will be inferred from the length of ``bvalues``. + + Yields + ------ + :obj:`int` The sorted index order. Examples @@ -54,7 +56,7 @@ def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]: if size is None: raise TypeError("Cannot build iterator without size") - return iter(range(size)) + return (s for s in range(size)) def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: @@ -68,14 +70,21 @@ def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` or None, optional - Number of volumes in the dataset (for instance, the number of - orientations in a DWI). If `None`, the size is inferred from the `bvals` - argument in `kwargs`, if present. - - Returns - ------- - :obj:`~typing.Iterator` + size : :obj:`int` or ``None`` (optional) + Number of volumes in the dataset. + bvalues : :obj:`list` (optional, keyword argument) + List of b-values corresponding to all orientations of a DWI dataset. + If ``size`` is provided, this argument will be ignored. + Otherwise, ``size`` will be inferred from the length of ``bvalues``. + seed : :obj:`int` or :obj:`bool` or :obj:`bool` or ``None`` (optional, keyword argument) + If :obj:`int` or :obj:`str` or ``None``, initializes the seed of Python's random generator + with the given value. + If ``False``, the random generator is passed ``None``. + If ``True``, a default seed value is set. + + Yields + ------ + :obj:`int` The sorted index order. Examples @@ -111,9 +120,14 @@ def bvalue_iterator(*_, **kwargs) -> Iterator[int]: """ Traverse the volumes in a DWI dataset by growing b-value. - Returns - ------- - :obj:`~typing.Iterator` + Parameters + ---------- + bvalues : :obj:`list` + List of b-values corresponding to all orientations of the dataset. + + Yields + ------ + :obj:`int` The sorted index order. Examples @@ -135,14 +149,16 @@ def centralsym_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` or None, optional - Number of volumes in the dataset (for instance, the number of - orientations in a DWI). If `None`, the size is inferred from the `bvals` - argument in `kwargs`, if present. - - Returns - ------- - :obj:`~typing.Iterator` + size : :obj:`int` or ``None`` (optional) + Number of volumes in the dataset. + bvalues : :obj:`list` (optional, keyword argument) + List of b-values corresponding to all orientations of the dataset. + If ``size`` is provided, this argument will be ignored. + Otherwise, ``size`` will be inferred from the length of ``bvalues``. + + Yields + ------ + :obj:`~int` The sorted index order. Examples From 4ebef0d45ca0fcbe1478484ae9b243a8d2b552ed Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 23 Jan 2025 09:34:00 +0100 Subject: [PATCH 4/5] enh: more accurate yield value documentation --- src/nifreeze/utils/iterators.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nifreeze/utils/iterators.py b/src/nifreeze/utils/iterators.py index 43a2bf12..beea0a25 100644 --- a/src/nifreeze/utils/iterators.py +++ b/src/nifreeze/utils/iterators.py @@ -43,7 +43,7 @@ def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Yields ------ :obj:`int` - The sorted index order. + The next index. Examples -------- @@ -85,7 +85,7 @@ def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Yields ------ :obj:`int` - The sorted index order. + The next index. Examples -------- @@ -128,7 +128,7 @@ def bvalue_iterator(*_, **kwargs) -> Iterator[int]: Yields ------ :obj:`int` - The sorted index order. + The next index. Examples -------- @@ -159,7 +159,7 @@ def centralsym_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Yields ------ :obj:`~int` - The sorted index order. + The next index. Examples -------- From 8ceaf57da4c1cd03d43a30e206a5a463b6dc1744 Mon Sep 17 00:00:00 2001 From: Oscar Esteban Date: Thu, 23 Jan 2025 15:58:27 +0100 Subject: [PATCH 5/5] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jon Haitz Legarreta Gorroño --- src/nifreeze/utils/iterators.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/nifreeze/utils/iterators.py b/src/nifreeze/utils/iterators.py index beea0a25..5205c9ec 100644 --- a/src/nifreeze/utils/iterators.py +++ b/src/nifreeze/utils/iterators.py @@ -33,9 +33,11 @@ def linear_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` or ``None`` (optional) + size : :obj:`int` or ``None``, optional Number of volumes in the dataset. - bvalues : :obj:`list` + If ``None``, ``size`` will be inferred from the ``bvalues`` + keyword argument. + bvalues : :obj:`list`, optional (keyword argument) List of b-values corresponding to all orientations of a DWI dataset. If ``size`` is provided, this argument will be ignored. Otherwise, ``size`` will be inferred from the length of ``bvalues``. @@ -63,20 +65,22 @@ def random_iterator(size: int | None = None, **kwargs) -> Iterator[int]: """ Traverse the dataset volumes randomly. - If the `seed` key is present in the keyword arguments, initializes the seed - of Python's `random` pseudo-random number generator library with the given - value. Specifically, if `False`, `None` is used as the seed; it `True`, a + If the ``seed`` key is present in the keyword arguments, initializes the seed + of Python's ``random`` pseudo-random number generator library with the given + value. Specifically, if ``False``, ``None`` is used as the seed; if ``True``, a default seed value is used. Parameters ---------- - size : :obj:`int` or ``None`` (optional) + size : :obj:`int` or ``None``, optional Number of volumes in the dataset. - bvalues : :obj:`list` (optional, keyword argument) + If ``None``, ``size`` will be inferred from the ``bvalues`` + keyword argument. + bvalues : :obj:`list`, optional (keyword argument) List of b-values corresponding to all orientations of a DWI dataset. If ``size`` is provided, this argument will be ignored. Otherwise, ``size`` will be inferred from the length of ``bvalues``. - seed : :obj:`int` or :obj:`bool` or :obj:`bool` or ``None`` (optional, keyword argument) + seed : :obj:`int`, :obj:`bool`, :obj:`str`, or ``None``, optional (keyword argument) If :obj:`int` or :obj:`str` or ``None``, initializes the seed of Python's random generator with the given value. If ``False``, the random generator is passed ``None``. @@ -124,6 +128,8 @@ def bvalue_iterator(*_, **kwargs) -> Iterator[int]: ---------- bvalues : :obj:`list` List of b-values corresponding to all orientations of the dataset. + Please note that ``bvalues`` is a keyword argument and MUST be provided + to generate the volume sequence. Yields ------ @@ -149,9 +155,11 @@ def centralsym_iterator(size: int | None = None, **kwargs) -> Iterator[int]: Parameters ---------- - size : :obj:`int` or ``None`` (optional) + size : :obj:`int` or ``None``, optional Number of volumes in the dataset. - bvalues : :obj:`list` (optional, keyword argument) + If ``None``, ``size`` will be inferred from the ``bvalues`` + keyword argument. + bvalues : :obj:`list`, optional (keyword argument) List of b-values corresponding to all orientations of the dataset. If ``size`` is provided, this argument will be ignored. Otherwise, ``size`` will be inferred from the length of ``bvalues``.