balbasty / nitorch

Neuroimaging in PyTorch
Other
86 stars 14 forks source link

remove duplicated offsets assignments #65

Closed wyli closed 2 years ago

wyli commented 2 years ago

looks like the offsets are computed twice...

balbasty commented 2 years ago

Hi Wenqi,

Sorry, I'd missed this PR. Many thanks for raising this!

The only concern I have is that in grid_pull_backward, we have both do_push = true and do_grad = true. I am afraid that if we compute offsets only once at the beginning, the push pass will use wrong offsets, since we'll enter in the first branch of the if/else statement. What do you think?

Currently, it goes:

- if (do_grad || do_pull || do_sgrad) { compute source offsets } else {compute out offsets }
- if (do_grad) { ... }
- if (do_pull) ... else if (do_sgrad) ... else if (do_push) ... else if (do_count) ...

I guess a solution would be to move the push/count offsets branch after if(do_grad):

- if (do_grad || do_pull || do_sgrad) { compute source offsets }
- if (do_grad) { ... }
- if (do_push || do_count) {compute out offsets }
- if (do_pull) ... else if (do_sgrad) ... else if (do_push) ... else if (do_count) ...
wyli commented 2 years ago

thanks @balbasty, I've updated the PR based on your suggestion.

balbasty commented 2 years ago

Thanks! This is minor, but you'll see that we now have a bunch of compilation warnings about uninitialized offsets. We cover all cases so this won't be an issue at runtime. In MONAI, you may want to dummy-initialize these offsets to avoid warnings (I can take care of it here). Thanks again :-)

wyli commented 2 years ago

I guess I managed to mute the warning, I'll update MONAI for the same... @balbasty many thanks for the review!