-
Notifications
You must be signed in to change notification settings - Fork 5
ENH: Improve general handling of parallelization #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
48c3453 to
d513d05
Compare
Brings improvements in parallelization management from #142, so they are kept even if we finally decided against #142. In particular, it opens up the implementation to set the ``step`` feature of DTI models. It also extends the base data object with two convenience properties for retrieving the 3D shape of the data and the number of voxels per volume. X-References: #142.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 70.10% 70.69% +0.58%
==========================================
Files 23 23
Lines 1067 1126 +59
Branches 129 135 +6
==========================================
+ Hits 748 796 +48
- Misses 275 284 +9
- Partials 44 46 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhlegarreta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of in-line comments to be addressed in separate PRs if we believe they make sense: I prefer to have this merged to move forward.
| def size3d(self): | ||
| """Get the number of voxels in the 3D volume.""" | ||
| return np.prod(self.dataobj.shape[:3]) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like better volume_shape and volume_size for the properties, but I prefer to have this merged and change them at a latter stage if we agree on better naming.
|
|
||
| print(f"Dataset size: {num_voxels}x{len(dataset)}.") | ||
| print(f"Parallel execution: {fit_pred_kwargs}.") | ||
| print(f"Model: {model}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that having proper logging instead of using print statements will be required in the mid-term.
Brings improvements in parallelization management from #142, so they are kept even if we finally decided against #142.
In particular, it opens up the implementation to set the
stepfeature of DTI models.It also extends the base data object with two convenience properties for retrieving the 3D shape of the data and the number of voxels per volume.
X-References: #142.