Hello@htwangtw, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally,pip install flake8 and then runflake8 nibabel.

Comment last updated at 2021-06-21 13:21:22 UTC

Codecov Report

Merging#1014 (fb3bcfc) intomaster (44a1052) willincrease coverage by0.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

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update44a1052...fb3bcfc. Read thecomment docs.

Hi@htwangtw, I'm interpreting theDRAFT 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:

1. Memory management: A normal image loaded bynibabel,dataobj is some reference to the data on disc. When we do an operation, thedataobj will be read and stored in the memory, and the same for the new nifti image generate. Am I missing anything?

2. ZeroDivisionError: To my surprise numpy returnsinf, notZeroDivisionError when dividing a value by zero. Is this a thing better be flagged as error before the operation or just let it be?

3. Behaviour when dealing with 3D vs 4D image: I don't have a good answer on this one. We can assume people are familiar withfslmaths and get similar image outputs. We can just disallow this kind of case, but I think it would be nice to havefslmaths like behaviour. Thoughts???

4. Operators that only require one input: I can seeabs(img),-img,+img really useful. I just don't have an idea how they can fit into the current_op method. 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:

1. Memory management: A normal image loaded bynibabel,dataobj is some reference to the data on disc. When we do an operation, thedataobj will 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 allowingArrayProxys to build up a sequence of partial applications, but that is a much bigger task than is called for here.

2. ZeroDivisionError: To my surprise numpy returnsinf, notZeroDivisionError when 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.

3. Behaviour when dealing with 3D vs 4D image: I don't have a good answer on this one. We can assume people are familiar withfslmaths and get similar image outputs. We can just disallow this kind of case, but I think it would be nice to havefslmaths like behaviour. Thoughts???

I'm not sure what thefslmaths 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?

4. Operators that only require one input: I can seeabs(img),-img,+img really useful. I just don't have an idea how they can fit into the current_op method. Thoughts?

You could do something like:

classOperableImage:def_binop(self,val,*,op):        ...def_unop(self,*,op):        ...

Hey@effigies -

I tried to add someNifti1Image.dataobj type test and realise theNifti1Image class itself has pretty comprehensive checks upon image creation. Can you think of a case that invalidNifti1Image.dataobj value can be loaded?