ENH: Refactor filters into filtering
#71
Conversation
|
A few comments:
|
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
from
5cec644 to
7e67f76
Compare
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
2 times, most recently
from
1170199 to
504ea30
Compare
src/nifreeze/data/filtering.py
Outdated
|
|
||
| clipped_dataset._dataset = clipped_dataset._dataset.dataobj[..., shellmask] | ||
|
|
||
| return clipped_dataset |
There was a problem hiding this comment.
If we clip the data, maybe the gradients stored in the datasets should also be clipped.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
- Coverage 69.28% 64.97% -4.31%
==========================================
Files 20 20
Lines 967 988 +21
Branches 119 121 +2
==========================================
- Hits 670 642 -28
- Misses 254 306 +52
+ Partials 43 40 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
3 times, most recently
from
82af75c to
7c8e9f1
Compare
|
Tests are failing because the exception has now been moved to: So this may requires some discussion. |
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
10 times, most recently
from
122ecc9 to
ceae0c2
Compare
|
@oesteban I would be grateful if you could review this, even if I have not marked as ready for review. Some comments:
|
| shellmask = np.ones(len(self._dataset), dtype=bool) | ||
|
|
||
| # Keep only bvalues within the range defined by th_high and th_low | ||
| shellmask[index] = False |
There was a problem hiding this comment.
@oesteban While writing the test, I have realized that this statement should be removed: we want to keep the value at the index position, right?
There was a problem hiding this comment.
I removed the statement. Consequently, the exception here is not not raised:
https://github.com/nipreps/nifreeze/pull/71/files#diff-2197e776366f74ac177f201dce923642554f122630fabab0b45063f2f6cf1832R230-R231
https://github.com/nipreps/nifreeze/actions/runs/14149369168/job/39640631654?pr=71#step:11:539
So I am wondering about the rationale behind assigning False to the index.
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
3 times, most recently
from
3656d49 to
66db806
Compare
| if mask is not None: | ||
| volumes = data[..., mask] |
There was a problem hiding this comment.
The check here is necessary otherwise a new, unnecessary dimension is added to the data.
|
Re #71 (comment):
|
Refactor code blocks that perform filtering operations within models into the `filtering` module so that the model and filter concepts are separated, supporting more cleanly pipelining models with filters within the new `Estimator` philosophy. Add the percentile-based detrending feature that was temporarily removed in commit 7322e93. Take advantage of the commit to rename the `th_low` and `th_high` attributes of the `AverageDWIModel` to `atol_low` and `atol_high`. Co-authored-by: Oscar Esteban <[email protected]>
jhlegarreta
force-pushed
the
RefactorFilteringCode
branch
from
66db806 to
e4819e4
Compare
Refactor code blocks that perform filtering operations within models into the
filteringmodule so that the model and filter concepts are separated, supporting more cleanly pipelining models with filters within the newEstimatorphilosophyAdd the percentile-based detrending feature that was temporarily removed in commit 7322e93.
Closes #63.