[FIX,REF] start changing how to handle resampling #439
Conversation
Codecov Report
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
+ Coverage 82.50% 82.56% +0.06%
==========================================
Files 40 40
Lines 3892 3907 +15
==========================================
+ Hits 3211 3226 +15
Misses 681 681
Continue to review full report at Codecov.
|
|
The docs failure seems to be the random bug documented in #418. I still don't have a solution, so I think we can just restart the CI and it might go away. |
nimare/base.py
Outdated
| self.resample = kwargs.get("resample", False) | ||
|
|
||
| # defaults for resampling images (nilearn's defaults do not work well) | ||
| self.resample_kwargs = { |
There was a problem hiding this comment.
Do we want to expose these to users? If not, maybe self._resample_kwargs? I forget what the conventions we're using are; if this is how we're doing it elsewhere, then it's fine this way.
There was a problem hiding this comment.
good catch! I do not think I want to expose this to users, and yes you are right that appears to be the convention, changed!
nimare/base.py
Outdated
|
|
||
| # defaults for resampling images (nilearn's defaults do not work well) | ||
| self.resample_kwargs = { | ||
| k.split("resample__")[1]: v for k, v in kwargs.items() if k.startswith("resample__") |
There was a problem hiding this comment.
I think the or here will force the user to specify both arguments even if they only want to override one default (and they may not realize that, which is likely to lead to unintended behavior). Probably better to create the default dict and then update it with any matching argument names in kwargs.
There was a problem hiding this comment.
great point, changed it so there is a default dict and arguments can update that dict.
nimare/base.py
Outdated
| if not self.resample: | ||
| check_imgs = {img: nb.load(img) for img in self.inputs_[name]} | ||
| check_imgs["reference_masker"] = mask_img | ||
| _check_same_fov(**check_imgs, raise_error=True) |
There was a problem hiding this comment.
It's pretty trivial, and would only slightly improve readability, but to save the del call later you could just pass reference_masker as an argument directly to _check_same_fov instead of adding it to check_imgs, since you remove it right away
There was a problem hiding this comment.
clean code is happy code :), changed.
- change resample_kwargs to _resample_kwargs - improve readability of _check_fov - update instead of overwrite defaults
Closes #438
Changes proposed in this pull request:
fitmethod or should we make it a part of initializing the estimator object so the user's decision is saved in a more permanent way? and to keep thefitmethod clean)