Add a new Image class which allows using images in GLSL

[robquill]

Create a Memory base-class which Image and Tensor inherit from.

[robquill]

General comments (from @axsaucedo)

Renaming of classes Tensor->Vec, Image->Mat: See below for context on thinking Let's discuss if doing renaming now or after merge Namely as reviewing renaming PR easier than together with all changes

height, weight -> x,y. Would there also be an Mat3D impl (in the future)?

Memory object: Parent class impl makes sense - naming convention, is Memory the best representation? Keen to explore this question as it will become core to framework What are alternatives considered? Why does Memory.hpp have #include ? Remember to prefix kp:: all Kompute objects even inside namespace (incl other files) We've been looking as an opportunity to expose the underlying buffer (so people can bring their own) However this to be approached as separate initiative / PR

Manager.hpp: Why different map to manage images vs vectors vs single mem one?

Tests: Why on tests changing TensorT->Memory? ie lose functionality from abstraction Let's run codecov to ensure we can look to have good coverage Test custom dtype for vectors vs image?

Operations: Currently requires explicitly specifying OpASync[...] or OpACopyB Is there a way to implement an OpCopy or OpSync that don't require explicitly selecting? I haven't properly thought about it being possible / not possible but wanted to ask as adds cognitive load on api (Relevant to question above of why changing Tensor->Memory on tests)

On the point for "A more recent issue asks for 8-bit data support": Currently adding an extra datatype should be pretty straightforward See https://github.com/KomputeProject/kompute/blob/a1555854bb035feb9f15c3049224d2f6dd961409/test/TestOpShadersFromStringAndFile.cpp#L12-L59 Is there something you meant further to just adding uint8_t to the supported datatypes? @SimLeek, on this point "Casting everything to float through python and back is basically a hack": I'm trying to understand what you mean, from the current impl. it would be using numpy dtypes, do you mean when converting to array? Also on this point "Oh, also, I kinda want custom data types as a different issue, like sending structs to buffers.": Does the link above answer the question or you meant something else?

[robquill]

TODO:

• [x] Update examples (they don't build)
• [x] height, weight -> x,y.
• [x] Let's run codecov to ensure we can look to have good coverage
• [x] Test custom dtype for vectors vs image?
• [ ] Update / extend the ref diagram (bottom right) as progresses: https://app.diagrams.net/#G1mRJlFEWpYD2dMU7j1HGwwDrxPeEmUjl8#%7B%22pageId%22%3A%22fwopQ7pWVPHFaTMxwvhy%22%7D
• [ ] Adding further python tests to identify issues/limits with tensor/image interactions
• [x] Explore/discuss exposing further functionality through top level parent class
• [ ] Implement or extend 1-2 of the more advanced examples with image class to further battle test the limitations (can be done as a separate PR)
• [x] Exploring how the height,width->x,y could also be exposed top level class (Tensor class could also have the same as default - i.e. x,y->x,1)

Answered:

• [x] Is there a test for having too many channels for an image buffer?

There is now.

• [x] numChannels doesn't seem to be described in docstrings, like in Image.hpp

Fixed.

• [x] Why on tests changing TensorT->Memory? ie lose functionality from abstraction

The reason is that now the Sequence->eval uses Memory instead of Tensor and it looks like the compiler doesn't know how to convert std::vector<std::shared_ptr<Tensor>> to std::vector<std::shared_ptr<Memory>>. For example, this snippet from TestASyncOperations.cpp does not work:

    std::vector<std::shared_ptr<kp::Tensor>> inputsSyncB;
    ...
    sq->eval<kp::OpTensorSyncDevice>(inputsSyncB);

because:

/home/rob/dev/git/kompute/test/TestAsyncOperations.cpp:61:38: note:   cannot convert ‘inputsSyncB’ (type ‘std::vector<std::shared_ptr<kp::Tensor> >’) to type ‘std::vector<std::shared_ptr<kp::Memory> >’

• [x] Why does Memory.hpp have #include ?

Because std::shared_ptr is defined there.

• [x] Remember to prefix kp:: all Kompute objects even inside namespace (incl other files)

Only required for tests.

• [x] Currently requires explicitly specifying OpASync[...] or OpACopyB. Is there a way to implement an OpCopy or OpSync that don't require explicitly selecting?

Done in the latest version. Now there is just OpSyncLocal and OpSyncDevice.

• [x] Manager.hpp: Why different map to manage images vs vectors vs single mem one?

Not required. Fixed.

Deferred:

• [ ] Renaming of classes Tensor->Vec, Image->Mat:
• [ ] naming convention, is Memory the best representation?
• [ ] We've been looking as an opportunity to expose the underlying buffer (so people can bring their own). However this to be approached as separate initiative / PR.
• [ ] Revisit how exceptions are used. Maybe use exception types other than std::runtime_error.

[robquill]

Test custom dtype for vectors vs image?

I'm not completely sure what this meant. Custom types don't make sense for images as you can only create them with the formats Vulkan/the HW supports. You can't have images that contain structs etc for example.

[axsaucedo]

Test custom dtype for vectors vs image?

I'm not completely sure what this meant. Custom types don't make sense for images as you can only create them with the formats Vulkan/the HW supports. You can't have images that contain structs etc for example.

Good point, it seems indeed the Image type would not support the dtype, may be worth covering this as a compile-type error if anyone tries

[robquill]

Test custom dtype for vectors vs image?

I'm not completely sure what this meant. Custom types don't make sense for images as you can only create them with the formats Vulkan/the HW supports. You can't have images that contain structs etc for example.

Good point, it seems indeed the Image type would not support the dtype, may be worth covering this as a compile-type error if anyone tries

I'm not sure how to make this a compile-time error because the dtype is passed as a variable.

[axsaucedo]

I'm not sure how to make this a compile-time error because the dtype is passed as a variable.

Apologies, it seems I didn't share full context. In the current implementation there's nothing that would have to be done. However following the suggestion here of merging the imageType and tensorType into a top level enum it would be required as it would by default have an eCustom enum value (as this is possible for the Tensor class), so for the image it would be required to restrict it, which could be done by using something like std::is_base_of.

However indeed it would only be necessary given the suggestion above - although hasn't yet been discussed. Let me know if this is not clear and I can expand.

[robquill]

I'm not sure how to make this a compile-time error because the dtype is passed as a variable.

Apologies, it seems I didn't share full context. In the current implementation there's nothing that would have to be done. However following the suggestion here of merging the imageType and tensorType into a top level enum it would be required as it would by default have an eCustom enum value (as this is possible for the Tensor class), so for the image it would be required to restrict it, which could be done by using something like std::is_base_of.

However indeed it would only be necessary given the suggestion above - although hasn't yet been discussed. Let me know if this is not clear and I can expand.

OK, let me see how much I can refactor into the Memory class then we can see what to do about this. I don't completely understand the purpose of merging Image and Tensor and having an enum to differentiate. You would just end up replacing dynamic_cast with a test of the enum, which I imagine is what the compiler ends up doing internally to do the dynamic_cast.

[axsaucedo]

I'm not sure how to make this a compile-time error because the dtype is passed as a variable.

Apologies, it seems I didn't share full context. In the current implementation there's nothing that would have to be done. However following the suggestion here of merging the imageType and tensorType into a top level enum it would be required as it would by default have an eCustom enum value (as this is possible for the Tensor class), so for the image it would be required to restrict it, which could be done by using something like std::is_base_of. However indeed it would only be necessary given the suggestion above - although hasn't yet been discussed. Let me know if this is not clear and I can expand.

OK, let me see how much I can refactor into the Memory class then we can see what to do about this. I don't completely understand the purpose of merging Image and Tensor and having an enum to differentiate. You would just end up replacing dynamic_cast with a test of the enum, which I imagine is what the compiler ends up doing internally to do the dynamic_cast.

Just to clarify, it wouldn't be merging the entire image and tensor class, but only the imageDataType and tensorDataType into a single enum DataType - although I assume that's what you meant. The main reason I was suggesting this is to allow for the Memory class to have a top level virutal Memory::DataType datatype() funcition that can then can be called directly through the parent class without having to convert, and hence being able to implement the respective operators.

In regards to the dynamic_cast, I have to say that overall I don't have big issues with this implementation, as I liked how it was used - however seeing the need for a dynamic cast makes me think that having an explicit memoryImplementation variable that specifies whether it's an image vs a tensor could be valuable so the check can be done explicitly instead of requiring a dynamic_cast in case users don't need to completely cast it to perform an operation.

[robquill]

I'm not sure how to make this a compile-time error because the dtype is passed as a variable.

Apologies, it seems I didn't share full context. In the current implementation there's nothing that would have to be done. However following the suggestion here of merging the imageType and tensorType into a top level enum it would be required as it would by default have an eCustom enum value (as this is possible for the Tensor class), so for the image it would be required to restrict it, which could be done by using something like std::is_base_of. However indeed it would only be necessary given the suggestion above - although hasn't yet been discussed. Let me know if this is not clear and I can expand.

OK, let me see how much I can refactor into the Memory class then we can see what to do about this. I don't completely understand the purpose of merging Image and Tensor and having an enum to differentiate. You would just end up replacing dynamic_cast with a test of the enum, which I imagine is what the compiler ends up doing internally to do the dynamic_cast.

Just to clarify, it wouldn't be merging the entire image and tensor class, but only the imageDataType and tensorDataType into a single enum DataType - although I assume that's what you meant. The main reason I was suggesting this is to allow for the Memory class to have a top level virutal Memory::DataType datatype() funcition that can then can be called directly through the parent class without having to convert, and hence being able to implement the respective operators.

In regards to the dynamic_cast, I have to say that overall I don't have big issues with this implementation, as I liked how it was used - however seeing the need for a dynamic cast makes me think that having an explicit memoryImplementation variable that specifies whether it's an image vs a tensor could be valuable so the check can be done explicitly instead of requiring a dynamic_cast in case users don't need to completely cast it to perform an operation.

OK, the latest code has imageDataType and tensorDataType replaced with a single enum Memory::DataType. As part of that I added support for Tensors of u/int8/16_t as these formats were already supported for images.

[robquill]

@axsaucedo , most of the major refactoring/consolidation is now done if you want to give this another review. There are some small bits like x,y still to do but it should be in a lot better shape now.

So far I don't think we need an enum for tensor/image but see what you think and let me know what you want to do (if anything) with custom dtypes for images.

[robquill]

Code coverage generally looks good (see attached). Most of the lines that aren't reached are because they are error cases that are difficult/impossible to hit. See the attached report. html.zip

[robquill]

@axsaucedo , does this updated diagram look OK to you? https://drive.google.com/file/d/1iKMY9YPKT97WgjYSYQ8LNHmRyXPwwx-H/view?usp=sharing

[robquill]

@axsaucedo , I've made a bit of a mess of the rebase by trying to fix the DCO. The end state should be unchanged but I'm thinking of just squashing all the commits into one as now the middle is a bit of a mess. Any thoughts/objections before I go ahead and do it?

[axsaucedo]

@robquill no objections, we would squash on merge otherwise