Skip to content
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

NF Operator class #1014

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

NF Operator class #1014

wants to merge 8 commits into from

Conversation

htwangtw
Copy link
Contributor

@htwangtw htwangtw commented May 6, 2021

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:

  1. Are there header/affine check functions that already exist in nibabel that I can reuse?
  2. 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

  • check any other operator we want to cover
  • better/flexible evaluation of data shape - I would use fslmaths as a reference dealing with masking 4D array with 3D image
  • more test cases
  • better error handling
  • docstring
  • warning related to caching

This is a draft for community feedback.
I would like to know
1. Are there header/affine check functions that already exist in nibabel
that I can reuse?
2. more efficient way to add more operators?

I have to admit I am not super familiar with a lot of tools related to
class object that comes with python.
@pep8speaks
Copy link

pep8speaks commented May 6, 2021

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 run flake8 nibabel.

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

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #1014 (fb3bcfc) into master (44a1052) will increase coverage by 0.03%.
The diff coverage is 98.46%.

Impacted file tree graph

@@            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%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44a1052...fb3bcfc. Read the comment docs.

@effigies
Copy link
Member

effigies commented May 7, 2021

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.

@htwangtw htwangtw changed the title DRAFT/NF Operator class NF Operator class May 12, 2021
@htwangtw htwangtw marked this pull request as ready for review May 12, 2021 20:34
@htwangtw
Copy link
Contributor Author

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 by nibabel, dataobj is some reference to the data on disc. When we do an operation, the dataobj 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 returns inf, not ZeroDivisionError 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 with fslmaths and get similar image outputs. We can just disallow this kind of case, but I think it would be nice to have fslmaths like behaviour. Thoughts???

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

@effigies
Copy link
Member

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 by nibabel, dataobj is some reference to the data on disc. When we do an operation, the dataobj 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 allowing ArrayProxys to build up a sequence of partial applications, but that is a much bigger task than is called for here.

  1. ZeroDivisionError: To my surprise numpy returns inf, not ZeroDivisionError 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.

  1. 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 fslmaths and get similar image outputs. We can just disallow this kind of case, but I think it would be nice to have fslmaths like 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?

  1. Operators that only require one input: I can see abs(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:

class OperableImage:
    def _binop(self, val, *, op):
        ...
    def _unop(self, *, op):
        ...

@htwangtw
Copy link
Contributor Author

htwangtw commented Jun 21, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement arithmetic and logical operator support?
3 participants