NF Operator class
Closes #939.
This is a draft for feedback. Any suggestion is welcome! I am not very familiar with python class so please let me know if there's things in python I haven't taken advantage of already. I would like to know:
- Are there header/affine check functions that already exist in
nibabelthat I can reuse? - More efficient way to add more operators? I don't quite understand the example in the original issue:
__and__ = partial(_op, op=operator.__and__)
To-do
- [x] check any other operator we want to cover
- [x] better/flexible evaluation of data shape - I would use
fslmathsas a reference dealing with masking 4D array with 3D image - [ ] more test cases
- [ ] better error handling
- [x] docstring
- [ ] warning related to caching
Hello @htwangtw, Thank you for updating!
Cheers! There are no style issues detected in this Pull Request. :beers: To test for issues locally, pip install flake8 and then run flake8 nibabel.
Comment last updated at 2021-06-21 13:21:22 UTC
Codecov Report
Merging #1014 (fb3bcfc) into master (44a1052) will increase coverage by
0.03%. The diff coverage is98.46%.
@@ Coverage Diff @@
## master #1014 +/- ##
==========================================
+ Coverage 92.26% 92.29% +0.03%
==========================================
Files 100 101 +1
Lines 12201 12265 +64
Branches 2134 2144 +10
==========================================
+ Hits 11257 11320 +63
Misses 616 616
- Partials 328 329 +1
| Impacted Files | Coverage Δ | |
|---|---|---|
| nibabel/arrayops.py | 98.41% <98.41%> (ø) |
|
| nibabel/nifti1.py | 92.12% <100.00%> (+0.01%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 44a1052...fb3bcfc. Read the comment docs.
Hi @htwangtw, I'm interpreting the DRAFT heading to mean you want to push on this more pre-review, but please just ping me whenever you want feedback, or if you have design questions you want to raise.
I found the draft function not particular meaningful to me so I just make it a normal PR... @effigies I have a few questions:
-
Memory management: A normal image loaded by
nibabel,dataobjis some reference to the data on disc. When we do an operation, thedataobjwill be read and stored in the memory, and the same for the new nifti image generate. Am I missing anything? -
ZeroDivisionError: To my surprise numpy returnsinf, notZeroDivisionErrorwhen dividing a value by zero. Is this a thing better be flagged as error before the operation or just let it be? -
Behaviour when dealing with 3D vs 4D image: I don't have a good answer on this one. We can assume people are familiar with
fslmathsand get similar image outputs. We can just disallow this kind of case, but I think it would be nice to havefslmathslike behaviour. Thoughts??? -
Operators that only require one input: I can see
abs(img),-img,+imgreally useful. I just don't have an idea how they can fit into the current_opmethod. Thoughts?
I found the draft function not particular meaningful to me so I just make it a normal PR... @effigies I have a few questions:
- Memory management: A normal image loaded by
nibabel,dataobjis some reference to the data on disc. When we do an operation, thedataobjwill be read and stored in the memory, and the same for the new nifti image generate. Am I missing anything?
Nope. No avoiding it. The only other way to do it would be by allowing ArrayProxys to build up a sequence of partial applications, but that is a much bigger task than is called for here.
ZeroDivisionError: To my surprise numpy returnsinf, notZeroDivisionErrorwhen dividing a value by zero. Is this a thing better be flagged as error before the operation or just let it be?
It's a valid IEEE754 value. You pays your money, you takes your chances.
- Behaviour when dealing with 3D vs 4D image: I don't have a good answer on this one. We can assume people are familiar with
fslmathsand get similar image outputs. We can just disallow this kind of case, but I think it would be nice to havefslmathslike behaviour. Thoughts???
I'm not sure what the fslmaths behavior is here, but I would expect if the volumetric dimensions match, to broadcast along the time dimension. I suspect this is the numpy default?
- Operators that only require one input: I can see
abs(img),-img,+imgreally useful. I just don't have an idea how they can fit into the current_opmethod. Thoughts?
You could do something like:
class OperableImage:
def _binop(self, val, *, op):
...
def _unop(self, *, op):
...
Hey @effigies -
I tried to add some Nifti1Image.dataobj type test and realise the Nifti1Image class itself has pretty comprehensive checks upon image creation. Can you think of a case that invalid Nifti1Image.dataobj value can be loaded?