Add a new Image class which allows using images in GLSL

[robquill]

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

Operations and Sequences now use the Memory base-class instead of the Tensor class.

OpTensorCopy has been renamed to OpCopy and supports copying between Tensors and Tensors, Tensors and Images, Images and Tensors and Images and Images.

OpTensorSyncDevice/Local has been renamed to OpSyncDevice/Local and supports synchronising both Tensors and Images.

[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

kompute/test/TestOpShadersFromStringAndFile.cpp

Lines 12 to 59 in a155585

	// Introducing custom struct that can be used for tensors
	struct TestStruct {
	float x;
	uint32_t y;
	int32_t z;
	// Creating an == operator overload for the comparison below
	bool operator==(const TestStruct rhs) const {
	return this->x == rhs.x && this->y == rhs.y && this->z == rhs.z;
	}
	};
	// Custom struct needs to be mapped the eCustom datatype
	template<>
	kp::Tensor::TensorDataTypes
	kp::TensorT<TestStruct>::dataType()
	{
	return Tensor::TensorDataTypes::eCustom;
	}
	TEST(TestShader, ShaderRawDataFromConstructorCustomDataType)
	{
	std::string shader(R"(
	#version 450
	layout (local_size_x = 1) in;
	layout(std140, binding = 0) buffer a {
	float ax;
	uint ay;
	int az;
	};
	layout(std140, binding = 1) buffer b {
	float bx;
	uint by;
	int bz;
	};
	void main() {
	bx = ax;
	by = ay;
	bz = az;
	}
	)");
	kp::Manager mgr;
	std::shared_ptr<kp::TensorT<TestStruct>> tensorA = mgr.tensorT<TestStruct>({ { 0.1, 2, 3} });
	std::shared_ptr<kp::TensorT<TestStruct>> tensorB = mgr.tensorT<TestStruct>({ { 0.0, 0, 0} });

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:

• Update examples (they don't build)
• height, weight -> x,y.
• Let's run codecov to ensure we can look to have good coverage
• 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
• 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)
• 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:

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

There is now.

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

Fixed.

• 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> to std::vector<std::shared_ptr>. 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> >’

• Why does Memory.hpp have #include ?

Because std::shared_ptr is defined there.

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

Only required for tests.

• 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.

• 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

[axsaucedo]

Thank you for the PR @robquill - I've done an initial pass and added a few comments, let me know if any thoughts or questions

A few more points to add to the actions:

• Adding further python tests to identify issues/limits with tensor/image interactions
• 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)
• 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)

I've also been thinking about the design and I do feel like exposing more functionality to the top level class makes sense so it can be used without losing any functionality (+ simplifying API), so it's possible to have full abstraction on the underlying objects. Thinking that this could also allow for the python interface to enable for similar top level abstraction with this base class.

[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.

[axsaucedo]

This looks good, overall quite clean as well without major modifications to the API, as it seems most examples work as is (only with the OpTensorX -> OpX change). I added a few comments but these are minor. We should be able to move to finalising the last few admin points (docs, codecov, etc) and wrapping up in order to merge. Thank you for the contribution!

Specifically for the custom dtypes for images, we can add a static_assert to ensure the imageT class implementation is restricted to the supported types, but let me know if other ideas.

[axsaucedo]

Indentation

[robquill]

Fixed

[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

[axsaucedo]

LGTM! We can update documentation as a separate PR + with respective naming convention - DCO check is missing so we can merge.

[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