Transformation convenience methods and Image/Adapter implications

[thijsdhollander]

So to follow up on the proposal to include extra transformation convenience methods in the Image class, and the potential implications for Adapters... These are the relevant posts that make up this discussion up to now:

https://github.com/MRtrix3/mrtrix3/pull/491#issuecomment-201113926 https://github.com/MRtrix3/mrtrix3/pull/491#issuecomment-201126704 https://github.com/MRtrix3/mrtrix3/pull/491#issuecomment-201135204 https://github.com/MRtrix3/mrtrix3/pull/491#issuecomment-201208579 https://github.com/MRtrix3/mrtrix3/pull/491#issuecomment-203380093

...and that's where we are now. Great stuff to let sink in over the weekend, I'd say. :smile:

[draffelt]

.4. remove Adapters as a user-visible means of accessing image data .5. remove the ability to use Adapters altogether.

I'm not keen on this. I know they are currently only used once or twice in filters. However, in the registration branch (soon to be merged), we use many of the adapters in 7 different locations. So I think in the long term they will be handy for other applications too. If we are serious about providing a convenient imaging library for future developers I think they definitely need to stay.

.3. downgrade Adapters to a lower status, essentially providing only the functionality they were designed to provide.

Since we can't anticipate what they will be used for, I think they need to be fully fledged ImageTypes.

So the question is how do we deal with adding transform convenience methods with the current adapter framework? For the record, I'm not 100% opposed to keeping things as they are, however if we can work out a way for it to work without melting anyone's CPU or taking all their RAM, I think it would be a nice feature.

As I'd mentioned in a previous post, some of these Adapters need to modify the transform. This means they would need to recompute all the derived matrices as well, and store them separately from their parent Image. As things stand, they already store their own transform_type, but they would need to store a fair bit more again if these changes were implemented, which would make them heavier objects.

I admit this is a bit annoying. Note there are currently only 4/13 adapters that modify transform. I don't think it would be that messy to have each adapter have 3 extra transform members and accessor methods. And I'm not convinced the words heavy vs lightweight, and 'run-time overhead' are relevant in this context. A transform_type is 96 bytes, and would never be copy constructed in a tight loop. That said, I do acknowledge it might cause issues if a new developer comes along and creates their own adapter that overrides the image2scanner transform, and does not compute and override the other 3 transforms. I guess this is just a trade off: developer convenience of having transforms and current scanner position on hand vs risk of making a mistake when creating a new adapter.

Remind me again why we can't have a Transform object in header.h? With helper functions in Transform to ensure the 4 transforms are kept internally consistent when modified? Is it just to keep the header object as close as possible to what is stored on disk? I don't think keeping the header lightweight is a valid point here either. Storing 3 extra transforms in the header on load will consume 0.00028% of a typical 2.5mm FOD image. Also they will not be copied when copy constructing images since the buffer/header is a shared reference. If we could do this, then the adapters could simply override a single Transform member.

[jdtournier]

OK, glad we agree we need to keep the adapters... Like I said, I only suggested this since it basically removes any hurdles to doing this, with relatively little impact on the existing codebase. But if you're about to push a bunch of stuff that relies on them, that's a no-go.

I also agree that if we keep them, they need to be safe to use. So we'd need to figure out a way to ensure the changes don't introduce performance problems.

So realistically, the only performance issue I can foresee would occur if someone were to instantiate one of these within the body of a loop. One vaguely defensible use-case is the use of the Adapter::Subset for a neighbourhood-based algorithm - means you can loop over the neighbourhood using the Loop methods directly. But this is admittedly a very rare use-case, and it's pretty easy to use regular for loops instead. So maybe I shouldn't worry so much about that. We'll just need to document that performance will suffer if these are instantiated within the loop functor... And I'll just have to shut up about keeping these structures lightweight.

So let's find a way to do this cleanly and easily. I'm not sure about adding this to the Header directly, not so much because of performance (even I have to admit it's not going to make that much difference), but because it's a read-write structure - so we need to be careful to update everything when the voxel size changes, etc. to maintain consistency. Not impossible, just a bit messy (very messy actually, you'd need to modify the .spacing() method to return a proxy object, to catch any modifications made to the first 3 dimensions and trigger an update of the transform). But on the other hand, if we do provide these methods for the Image and Adapters, it really would be odd not to do so for the Header also.

Another option is to provide the same methods, but the Header version would return either a new matrix computed on the fly (so inherently up to date at that point), or check whether an update is needed prior to returning a const reference (essentially compare the voxel sizes and transform matrix with cached versions). The latter would be faster on repeated use, the former would be cleaner to implement and probably have very minimal impact on real-world performance - assuming users don't use the Header version of these calls within their main processing loop, which thankfully seems unlikely...

So I guess it can be done, and shouldn't be ridiculously complicated. @draffelt, do you want to give this a go...?

[draffelt]

Yep, I'm happy to tackle this one. Unfortunately it will probably have to wait until after ISMRM.