Modifications in the (bitbucket) tracking scripts - part 2 #511

EmmaRenauld · 2021-10-26T18:44:22Z

Bigger changes:

Merging the tracker and the propagator into propagator to avoid circular reference. Old file tracker.py (containing the two classes) becomes propagator.py
Adding a main script scil_compute_local_tracking_2.py (other names suggested?)
New "Tracker" class
- Englobes the track() function that was previously alone in local_tracking.py. This will allow creating a child version in dwi_ml. File renamed tracker.py
- Now the main script is: 1) Initialize the tracker. 2) Tracker.track().
- So to be clear, this new "Tracker" is absolutely not the same as the old, it is one step above. See new diagram:

https://docs.google.com/presentation/d/1mvvBYjKZ-IVmkl1EYWBTQ_8T8H0R3_Y-/edit?usp=sharing&ouid=101323301275554058305&rtpof=true&sd=true

Smaller changes:

Encapsulating parts in the scil_compute_local_tracking that are common in scilpy.tracking.utils
- Similiar to functions like add_processes_arg in scilpy.io.utils.
- It makes the proof-reading a bit less natural but at least the args are the same in both scripts. Do we like that? Separated in a few sub-functions so I can use only some of them in dwi_ml. If we prefer, I can copy all of this back into each script.
Now the seedGenerator and DataVolume take receive the data instead of the img. In dwi_ml, we have the data and affine, not the image, it would be a little weird to create the image back just to re-divide it.
In the DataVolume: renaming methods as in the SFT nomenclature. Ex: using "voxmm" instead of "position", to clarify. Adding "origin" option (center or corner)
AbstractTrackingField created above the ODFTrackingField.

pep8speaks · 2021-10-26T18:44:27Z

Hello @EmmaRenauld, Thank you for updating !

In the file scripts/scil_compute_local_tracking.py:

Line 131:80: E501 line too long (80 > 79 characters)

Comment last updated at 2021-12-20 18:27:16 UTC

scil-admin · 2021-10-26T19:14:05Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-10-26T19:46:58Z

Build passed ! Good Job 🍻 !

CHrlS98

Cool! Some comments!

scilpy/tracking/propagator.py

CHrlS98 · 2021-10-27T16:15:19Z

scilpy/tracking/local_tracking.py

-
-                pool = multiprocessing.Pool(nbr_processes,
-                                            initializer=_init_sub_process,
+                np.save(data_file_name,


why don't we simply pass the dataset in arguments to all threads? memory limitations? The whole mmap_mode thingy is quite mysterious to me.

Me too... I've left some #toDo for this, thinking we could deal with that later but if someone tells me what to do I can change it.

CHrlS98 · 2021-10-27T16:18:12Z

scilpy/tracking/local_tracking.py

+        global data_file_info
+
+        # args[0] is the Tracker.
+        self.propagator.tracking_field.dataset.data = np.load(


dataset is reloaded inside every thread anyway so I don't understand why it is not passed as an argument.

scilpy/tracking/local_tracking.py

scilpy/tracking/tracking_field.py

scripts/scil_compute_local_tracking_2.py

scilpy/tracking/local_tracking.py

scilpy/tracking/tracking_field.py

scripts/scil_compute_local_tracking.py

scil-admin · 2021-10-27T20:06:26Z

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-511/3/

scil-admin · 2021-10-27T20:46:40Z

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-511/4/

scil-admin · 2021-10-27T21:15:36Z

Build Failed 💥
https://jenkins.scil.usherbrooke.ca/job/scilpy/job/PR-511/5/

scil-admin · 2021-11-01T18:29:32Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-01T18:46:32Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-15T20:39:23Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-15T21:02:04Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-15T21:11:26Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-16T00:31:48Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-16T01:23:59Z

Build passed ! Good Job 🍻 !

CHrlS98

Ok, my last comments.

CHrlS98 · 2021-11-22T18:05:41Z

scilpy/image/datasets.py

@@ -84,7 +85,7 @@ def is_voxel_in_bound(self, i, j, k):
        return (0 <= i < self.dim[0] and 0 <= j < self.dim[1] and
                0 <= k < self.dim[2])

-    def get_voxel_at_position(self, x, y, z):
+    def voxmm_to_idx(self, x, y, z, origin='center'):


Is the new origin argument useful? Why would we need this kind of flexibility? Could we not just set it to one or the other and take it into account when writing our tracking scripts?

That dataset is not necessarily only associated with our traking script, though. It could eventually be used elsewhere, so I tought it was better to be thorough, like in the sft. But I won't object if others agree.

Yes ok. But in this case we need to be extra careful when writing our tracking scripts, because the SeedGenerator always returns positions with origin corner. Should the SeedGenerator have an origin argument too then?

I added one. For now it is not used-defined (self.origin='corner') but at least a bit more obvious?

scilpy/tracking/seed.py

scil-admin · 2021-11-22T18:23:54Z

Build passed ! Good Job 🍻 !

CHrlS98 · 2021-11-22T18:35:15Z

There is a subject that should still be discussed I think. This version works, but in the long term, I don't know if it's always going to fit with any model. I think that eventually, the tracking field should also be merged in the propagator.

Currently the tracking field: 1) gets the data (ex, fODF-sh), 2) returns the directions/maxima that are in a cone of angle theta. Then, the propagator sample from these directions.

But let's take the example of DTI. Sampling would be done direction on the tensor. I don't think we can really take a list of directions that are in a cone theta and sample on these directions, can we? So sampling should entirely be left to the propagator, and sampling should mean either, 'sampling from model' or 'sampling from a list of direction' based on the type of data. And it the case of DTI, theta is a stopping criterion, not a restreint on the tracking field.

Should we merge the tracking field and propagator? (Possibly in a 3rd PR)

Maybe we can wait for a specific situation where this refactoring is needed and do it if and when we need it, so that the new design really suits our requirements. Also I think deterministic tracking with DTI would work, because peaks are extracted on the whole sphere before selecting the direction inside a cone (if I'm not mistaken).

EmmaRenauld · 2021-11-22T18:39:01Z

Maybe we can wait for a specific situation where this refactoring is needed and do it if and when we need it, so that the new design really suits our requirements. Also I think deterministic tracking with DTI would work, because peaks are extracted on the whole sphere before selecting the direction inside a cone (if I'm not mistaken).

Well I actually need now to decide if I will try to force dwi_ml to fit with all this or not. In our case, the tracking_field gets the inputs at current position and runs it through the model. The output from the model is not necessarily a list of directions. It could be, ex, a learned tensor, which is the same as my DTI example above.

But right now, I can still use scilpy's propagator. It's just that my "tracking_field.get_possible_next_directions" actually returns the tensor, and the propagator _sample_next_direction samples from that tensor, and I will add theta back later as a stopping critera.

CHrlS98 · 2021-11-22T18:57:00Z

Will dwi_ml be incorporated to scilpy (if so the changes can be done in its associated PR), or will the scilpy modules be imported in another project?

I think it would be better to keep the PR as is and merge it before making new changes, cause this is already a nice refactor that could benefit others right away.

Also shouldn't there be a test for the new script?

scil-admin · 2021-11-22T18:58:12Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-22T19:14:44Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-22T19:29:51Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-11-22T19:46:28Z

Build passed ! Good Job 🍻 !

mdesco

It is still crashing when I run:
scil_compute_local_tracking_dev.py sub-197348__fodf.nii.gz seeds_fa015.nii.gz seeds_fa015.nii.gz test.trk --processes 8

Data to test:
https://www.dropbox.com/s/a4s8gvkbro4pqz3/PR_511.zip?dl=0

EmmaRenauld · 2021-11-23T17:41:01Z

Sorry @mdesco

Tested and retested with your data and mine, verified origin everywhere.

scil-admin · 2021-11-23T18:12:02Z

Build passed ! Good Job 🍻 !

…king

scil-admin · 2021-12-10T20:48:59Z

Build passed ! Good Job 🍻 !

CHrlS98 · 2021-12-20T18:17:02Z

@EmmaRenauld @GuillaumeTh
Ok just to summarize what is left to be done :

Add a test for the new script (see test_compute_local_tracking.py for example)
Let's discuss this comment : Modifications in the (bitbucket) tracking scripts - part 2 #511 (comment)

CHrlS98

Alright nice!

I will open a PR in the future to clarify the DataVolume, SeedGenerator coordinate system extravaganza.

scil-admin · 2021-12-20T18:53:07Z

Build passed ! Good Job 🍻 !

scil-admin · 2021-12-20T19:29:20Z

Build passed ! Good Job 🍻 !

EmmaRenauld marked this pull request as draft October 26, 2021 18:45

scil-admin requested a review from arnaudbore October 26, 2021 19:14

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 21bfea0 to 3b56c38 Compare October 26, 2021 19:17

CHrlS98 reviewed Oct 27, 2021

View reviewed changes

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 4cabe7c to 478373f Compare October 27, 2021 20:19

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 478373f to 5878dad Compare November 1, 2021 17:59

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch 2 times, most recently from 02a1e84 to 084947c Compare November 15, 2021 20:33

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 084947c to 8003c44 Compare November 16, 2021 00:00

EmmaRenauld added 8 commits November 16, 2021 10:38

Created abstract tracking_field class

6d3fa35

Merging propagator and tracker

f0eae1a

Creating new bigger Tracker class

997a3de

Cleaning code: forward and backward set to None was impossible

19229b9

Cleaning params

a21ff4b

Modifs to fit with dwi_ml

407e370

Adding a main script v2 + encapsulating parts in common with v1.

4f2c636

Dealing with space and origin in DataVolume and Seeds

8d489c3

Improving docstrings and other small fixes

d71ec83

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from f3065be to d71ec83 Compare November 22, 2021 17:50

CHrlS98 reviewed Nov 22, 2021

View reviewed changes

scil-admin requested a review from arnaudbore November 22, 2021 18:23

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 7b44376 to 1f3dd74 Compare November 22, 2021 18:54

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 1f3dd74 to 8db4416 Compare November 22, 2021 19:00

mdesco reviewed Nov 23, 2021

View reviewed changes

EmmaRenauld added 2 commits November 23, 2021 12:40

Facilitating inclusion with dwi_ml

a2c1223

Making sure origin is coherent everywhere

dd8c66b

EmmaRenauld force-pushed the Merging_propagator_and_tracker branch from 8db4416 to dd8c66b Compare November 23, 2021 17:40

Adding possibility to reset params in propagator before backward trac…

ba7fc94

…king

Adding default seed for easier reproducibility

c143fb7

Added test file

6356f14

CHrlS98 approved these changes Dec 20, 2021

View reviewed changes

GuillaumeTh merged commit 30b596c into scilus:master Jan 5, 2022

EmmaRenauld deleted the Merging_propagator_and_tracker branch January 7, 2022 16:21

Modifications in the (bitbucket) tracking scripts - part 2 #511

Modifications in the (bitbucket) tracking scripts - part 2 #511

Conversation

EmmaRenauld commented Oct 26, 2021 • edited Loading

pep8speaks commented Oct 26, 2021 • edited Loading

Comment last updated at 2021-12-20 18:27:16 UTC

scil-admin commented Oct 26, 2021

scil-admin commented Oct 26, 2021

CHrlS98 left a comment

Choose a reason for hiding this comment

CHrlS98 Oct 27, 2021

Choose a reason for hiding this comment

EmmaRenauld Oct 27, 2021

Choose a reason for hiding this comment

CHrlS98 Oct 27, 2021

Choose a reason for hiding this comment

scil-admin commented Oct 27, 2021

scil-admin commented Oct 27, 2021

scil-admin commented Oct 27, 2021

scil-admin commented Nov 1, 2021

scil-admin commented Nov 1, 2021

scil-admin commented Nov 15, 2021

scil-admin commented Nov 15, 2021

scil-admin commented Nov 15, 2021

scil-admin commented Nov 16, 2021

scil-admin commented Nov 16, 2021

CHrlS98 left a comment

Choose a reason for hiding this comment

CHrlS98 Nov 22, 2021

Choose a reason for hiding this comment

EmmaRenauld Nov 22, 2021

Choose a reason for hiding this comment

CHrlS98 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

EmmaRenauld Dec 20, 2021

Choose a reason for hiding this comment

scil-admin commented Nov 22, 2021

CHrlS98 commented Nov 22, 2021

EmmaRenauld commented Nov 22, 2021 • edited Loading

CHrlS98 commented Nov 22, 2021 • edited Loading

scil-admin commented Nov 22, 2021

scil-admin commented Nov 22, 2021

scil-admin commented Nov 22, 2021

scil-admin commented Nov 22, 2021

mdesco left a comment

Choose a reason for hiding this comment

EmmaRenauld commented Nov 23, 2021

scil-admin commented Nov 23, 2021

scil-admin commented Dec 10, 2021

CHrlS98 commented Dec 20, 2021

CHrlS98 left a comment

Choose a reason for hiding this comment

scil-admin commented Dec 20, 2021

scil-admin commented Dec 20, 2021

EmmaRenauld commented Oct 26, 2021 •

edited

Loading

pep8speaks commented Oct 26, 2021 •

edited

Loading

CHrlS98 Nov 22, 2021 •

edited

Loading

EmmaRenauld commented Nov 22, 2021 •

edited

Loading

CHrlS98 commented Nov 22, 2021 •

edited

Loading