ENH: Indexes sorting options #150

esavary · 2024-04-02T14:40:48Z

Introduce iterators to rearrange the indexes used in logo_split. Sorting options include linear, random, in ascending order following gradient magnitude, and central symmetric.

closes #111

jhlegarreta · 2024-04-03T14:40:47Z

src/eddymotion/estimator.py


        """
+
+        def handle_gradient_ordering():


Re: #149 (comment)

All the methods here do not access any instance variable, and any IDE will suggest them becoming static, as they were prior to 8153649. However the actions dictionary was triggering errors because it was taking static methods. All this could be avoided if these methods were refactored into a utils.py (or similar) module that takes care of sorting the DWI volumes. In fact, (i) the estimator should not take care of this at all: it should receive the DWI volumes already sorted, and (ii) these methods may be useful elsewhere.

Related to this, the strategies could be put into an enum so that it becomes clear which are the sorting possibilities, and they are documented and available at a single place. However, I know that using enums is not very popular across many major scientific Python packages.

Sounds like a great idea! I agree, I'll externalize the functions into a new module, util.py. Thanks for the suggestion!

oesteban

Some initial comments

src/eddymotion/utils.py

oesteban · 2024-04-04T14:42:11Z

src/eddymotion/utils.py

+    Parameters:
+    dwdata : :obj:`~eddymotion.dmri.DWI`
+        DWI dataset, represented by this tool's internal type.
+
+    Returns:
+    index_order : :obj:`numpy.ndarray`
+    The sorted index order.


Suggested change

Parameters:

dwdata : :obj:`~eddymotion.dmri.DWI`

DWI dataset, represented by this tool's internal type.

Returns:

index_order : :obj:`numpy.ndarray`

The sorted index order.

Parameters

----------

size : :obj:`int`

Number of volumes in dataset

(for instance, number of orientations in a DWI)

Returns

-------

:obj:`list` of :obj:`int`

The sorted index order.

Examples

--------

>>> linear_action(10)

[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

src/eddymotion/utils.py

test/test_splitting.py

test/test_model.py

src/eddymotion/utils.py

Co-authored-by: Oscar Esteban <[email protected]>

src/eddymotion/estimator.py

src/eddymotion/utils.py

Co-authored-by: Oscar Esteban <[email protected]>

src/eddymotion/utils.py

Co-authored-by: Oscar Esteban <[email protected]>

esavary · 2024-04-08T11:49:32Z

src/eddymotion/utils.py

+
+    Examples
+    --------
+    >>> random_iterator(15, seed=0)  # seed is 0


This test is failing on my computer locally. I'm getting a different value compared to yours @oesteban . Specifically, I'm getting [2, 11, 3, 10, 0, 4, 7, 5, 14, 12, 6, 9, 13, 8, 1]. Any idea why?

No idea - I might have copied the wrong order.

esavary · 2024-04-08T11:50:22Z

src/eddymotion/utils.py

+    [12, 2, 7, 8, 14, 4, 9, 13, 6, 3, 5, 0, 1, 10, 11]
+    >>> random_iterator(15, seed=True)  # seed is the default value 20210324
+    [13, 2, 8, 12, 10, 4, 9, 3, 0, 1, 14, 6, 11, 7, 5]
+    >>> random_iterator(15, seed=42)  # seed is 42


Same here, I obtain [6, 7, 9, 3, 0, 11, 14, 10, 5, 2, 4, 12, 1, 13, 8].

oesteban

Suggestions to avoid numpy for the time being. Also make the random range iterator a generator.

src/eddymotion/utils.py

oesteban · 2024-04-08T11:57:21Z

Suggestions to avoid numpy for the time being. Also make the random range iterator a generator.

These for sure will make the test values invalid - we'll need to re-write them.

Co-authored-by: Oscar Esteban <[email protected]>

esavary · 2024-04-08T12:12:51Z

Suggestions to avoid numpy for the time being. Also make the random range iterator a generator.

These for sure will make the test values invalid - we'll need to re-write them.

I changed them in ed20ca8

oesteban

Looking good, I just have a few suggestions to improve the docstrings.

If you want to make this PR perfect, I'd go for type annotations on the four functions involved.

In the meantime, perhaps open a separate PR (make sure to branch from master, not from here) addressing #151? That PR should be fairly fast to merge and then we can update this one to resolve the ruff complaints.

src/eddymotion/utils.py

Co-authored-by: Oscar Esteban <[email protected]>

oesteban

The assistance tool for adding type annotations has introduced some problems (moved the imports to the top, before the license banner) and modified some docstrings that were better before.

src/eddymotion/utils.py

oesteban · 2024-04-08T13:26:45Z

src/eddymotion/utils.py

-import random
-from itertools import chain, zip_longest
-


Revert the removal of imports and add the Generation import here:

Suggested change

import random

from itertools import chain, zip_longest

import random

from itertools import chain, zip_longest

from typing import Generator

oesteban · 2024-04-08T13:28:07Z

src/eddymotion/utils.py

+import random
+from itertools import chain, zip_longest
+from typing import Generator
+
 # emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-


Please, be careful, why these four lines here before the header?

Suggested change

import random

from itertools import chain, zip_longest

from typing import Generator

# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-

# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-

I fixed the imports in 9bda539.

Co-authored-by: Oscar Esteban <[email protected]>

src/eddymotion/utils.py

oesteban · 2024-04-08T13:51:59Z

src/eddymotion/utils.py

+import random
+from itertools import chain, zip_longest
+from typing import Generator
+
+# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
+# vi: set ft=python sts=4 ts=4 sw=4 et:
+#
+# Copyright 2024 The NiPreps Developers <[email protected]>
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# We support and encourage derived works from this project, please read
+# about our expectations at
+#
+#     https://www.nipreps.org/community/licensing/
+#
+"""Iterators to traverse the volumes in a 4D image."""


Suggested change

import random

from itertools import chain, zip_longest

from typing import Generator

# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-

# vi: set ft=python sts=4 ts=4 sw=4 et:

#

# Copyright 2024 The NiPreps Developers <[email protected]>

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

#

# We support and encourage derived works from this project, please read

# about our expectations at

#

# https://www.nipreps.org/community/licensing/

#

"""Iterators to traverse the volumes in a 4D image."""

# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-

# vi: set ft=python sts=4 ts=4 sw=4 et:

#

# Copyright 2024 The NiPreps Developers <[email protected]>

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

#

# We support and encourage derived works from this project, please read

# about our expectations at

#

# https://www.nipreps.org/community/licensing/

#

"""Iterators to traverse the volumes in a 4D image."""

import random

from itertools import chain, zip_longest

from typing import Iterator

src/eddymotion/utils.py

Co-authored-by: Oscar Esteban <[email protected]>

oesteban · 2024-04-08T14:12:16Z

Let's re-sync with main:

Check you have the main repo as a remote:

git remote -v

And if not, add it:

git remote add upstream [email protected]:nipreps/eddymotion.git

Fetch the latest:
```
git fetch upstream
```
Merge main into this branch:
```
git merge upstream/main
```

esavary requested a review from oesteban April 2, 2024 14:52

esavary changed the title ~~ENH: Indexes sorting options~~ ENH: Indexes sorting options -WIP Apr 2, 2024

esavary removed the request for review from oesteban April 2, 2024 14:57

esavary changed the title ~~ENH: Indexes sorting options -WIP~~ ENH: Indexes sorting options Apr 3, 2024

esavary requested review from oesteban and effigies April 3, 2024 09:19

jhlegarreta reviewed Apr 3, 2024

View reviewed changes

esavary changed the title ~~ENH: Indexes sorting options~~ ENH: Indexes sorting options -WIP Apr 4, 2024

esavary force-pushed the index-sorting branch from 4d7ec53 to 3343f34 Compare April 4, 2024 14:08

esavary added 4 commits April 4, 2024 16:13

enh: externalize sorting functions

3343f34

fix:import statement

1e305d9

sty: fix style error

6a8a585

sty: fix style error

4e199a9

oesteban reviewed Apr 4, 2024

View reviewed changes

sty: fix style errors

b13c975

oesteban reviewed Apr 4, 2024

View reviewed changes

esavary and others added 6 commits April 5, 2024 10:24

Apply suggestions from code review

148bb71

Co-authored-by: Oscar Esteban <[email protected]>

fix: revert stylistic changes

5155f9f

fix: revert stylistic changes

9420ce5

fix: revert stylistic changes

3d7afb1

Update src/eddymotion/utils.py

69611fa

Co-authored-by: Oscar Esteban <[email protected]>

Update src/eddymotion/utils.py

31fa9d6

Co-authored-by: Oscar Esteban <[email protected]>

oesteban reviewed Apr 5, 2024

View reviewed changes

src/eddymotion/estimator.py Outdated Show resolved Hide resolved

oesteban reviewed Apr 5, 2024

View reviewed changes

src/eddymotion/utils.py Outdated Show resolved Hide resolved

esavary and others added 2 commits April 5, 2024 11:39

Update src/eddymotion/utils.py

6320f66

Co-authored-by: Oscar Esteban <[email protected]>

fix: revert changes in estimator.py

4998afa

oesteban reviewed Apr 5, 2024

View reviewed changes

src/eddymotion/utils.py Outdated Show resolved Hide resolved

src/eddymotion/utils.py Outdated Show resolved Hide resolved

src/eddymotion/utils.py Outdated Show resolved Hide resolved

oesteban reviewed Apr 5, 2024

View reviewed changes

src/eddymotion/utils.py Outdated Show resolved Hide resolved

esavary and others added 2 commits April 5, 2024 13:05

Apply suggestions from code review

a424582

Co-authored-by: Oscar Esteban <[email protected]>

fix: typos

88bea1d

esavary commented Apr 8, 2024

View reviewed changes

oesteban reviewed Apr 8, 2024

View reviewed changes

Apply suggestions from code review

4163b0b

Co-authored-by: Oscar Esteban <[email protected]>

fix: random generator test results

ed20ca8

esavary requested a review from oesteban April 8, 2024 12:30

oesteban reviewed Apr 8, 2024

View reviewed changes

esavary and others added 3 commits April 8, 2024 14:51

Apply suggestions from code review

d966c97

Co-authored-by: Oscar Esteban <[email protected]>

sty: add typing

0057079

fix: remove unused import

6f0d5fe

oesteban reviewed Apr 8, 2024

View reviewed changes

Apply suggestions from code review

2cc923a

Co-authored-by: Oscar Esteban <[email protected]>

esavary commented Apr 8, 2024

View reviewed changes

src/eddymotion/utils.py Show resolved Hide resolved

oesteban reviewed Apr 8, 2024

View reviewed changes

src/eddymotion/utils.py Outdated Show resolved Hide resolved

esavary and others added 2 commits April 8, 2024 15:57

fix: import position

9bda539

Apply suggestions from code review

d1c9846

Co-authored-by: Oscar Esteban <[email protected]>

esavary added 3 commits April 8, 2024 16:15

fix: iterator typing in docstring

807a468

sty: fix ruff errors

fee865d

Merge branch 'nipreps:main' into index-sorting

5f0837d

esavary requested a review from oesteban April 8, 2024 14:28

esavary added 2 commits April 8, 2024 16:28

sty: fix sty errors

caa72a5

sty: fix sty errors

911a730

oesteban approved these changes Apr 8, 2024

View reviewed changes

oesteban merged commit 6128b37 into nipreps:main Apr 8, 2024
7 of 8 checks passed

jhlegarreta mentioned this pull request Apr 13, 2024

ENH: Call the outsourced random volume index iterator in estimator #165

Merged

jhlegarreta mentioned this pull request Dec 22, 2024

ENH: Fix type annotations in b-vals iterators nipreps/nifreeze#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Indexes sorting options #150

ENH: Indexes sorting options #150

esavary commented Apr 2, 2024 •

edited

Loading

jhlegarreta Apr 3, 2024

esavary Apr 4, 2024

oesteban left a comment

oesteban Apr 4, 2024

esavary Apr 8, 2024

oesteban Apr 8, 2024

esavary Apr 8, 2024

oesteban left a comment

oesteban commented Apr 8, 2024

esavary commented Apr 8, 2024

oesteban left a comment

oesteban left a comment

oesteban Apr 8, 2024

oesteban Apr 8, 2024

esavary Apr 8, 2024

oesteban Apr 8, 2024

oesteban commented Apr 8, 2024

ENH: Indexes sorting options #150

ENH: Indexes sorting options #150

Conversation

esavary commented Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

oesteban commented Apr 8, 2024

esavary commented Apr 8, 2024

oesteban left a comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban commented Apr 8, 2024

esavary commented Apr 2, 2024 •

edited

Loading