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

first version of OpenCL support for operations on rasters (and soon f… #32

Merged
merged 39 commits into from
Oct 12, 2020

Conversation

michaelmattig
Copy link
Contributor

…eature collections)

Co-authored-by: Christian Beilschmidt beilschmidt@mathematik.uni-marburg.de

michaelmattig and others added 2 commits September 8, 2020 17:08
…eature collections)

Co-authored-by: Christian Beilschmidt <beilschmidt@mathematik.uni-marburg.de>
operators/src/error.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
@michaelmattig
Copy link
Contributor Author

Unfortunately there is an issue that results in tests failing when they are run in parallel. I created an issue cogciprocate/ocl#189

@michaelmattig
Copy link
Contributor Author

Unfortunately there is an issue that results in tests failing when they are run in parallel. I created an issue cogciprocate/ocl#189

workaround: b0179ca

@michaelmattig michaelmattig marked this pull request as ready for review September 24, 2020 15:15
@michaelmattig
Copy link
Contributor Author

I implemented functionality for computing opencl kernels on Rasters and MultiPointCollections (and an example expression operator).

Still missing functionality:

  • Support for lines and polygons
  • Support for time intervals

There is also a lot of room for improvement for code conciseness and performance.

There is also a problem where sometimes arrow buffers are not correctly allocated. It happens sometimes when executing the tests multiple times in successions.

Please give me some feedback on the work and input on how we can limit the scope of this pull request s.t. it can be merged eventually.

Copy link
Member

@ChristianBeilschmidt ChristianBeilschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good so far. You missed at your "missing out…" list that the expression is limited to two rasters for now.

Otherwise, I found no major issues to not merge a first version, but still have some comments that should be addressed first.

operators/src/opencl/mod.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/opencl/cl_program.rs Show resolved Hide resolved
operators/src/opencl/cl_program.rs Outdated Show resolved Hide resolved
operators/src/processing/expression.rs Outdated Show resolved Hide resolved
operators/src/processing/expression.rs Show resolved Hide resolved
operators/src/processing/expression.rs Show resolved Hide resolved
operators/src/processing/expression.rs Outdated Show resolved Hide resolved
operators/src/processing/expression.rs Show resolved Hide resolved
@jdroenner
Copy link
Member

there are multiple things which we should discuss:

  • is the opencl code only for expression?
  • if not, what about operations which require overlapping on tile borders? Will this be possible with this approach?
  • if not, what about numeric parameters? How will i get them to a kernel?
  • Is the macro approach really scalable? Why not introduce data type changing adapters? they will never be more expensive as any other operator..

@michaelmattig
Copy link
Contributor Author

is the opencl code only for expression?

no, then we wouldn't need support for feature collections. it's meant as generic mechanism for transfering data for opencl processing s.t. not every operators need to performt the mapping of data

if not, what about operations which require overlapping on tile borders? Will this be possible with this approach?

handling overlapping tiles is responsibility of the opencl kernel code and the operator that feeds the data into the cl program. I don't see any inherent limitations in my code as It's solely responsible for encapsulating the transfer to and from the opencl device

if not, what about numeric parameters? How will i get them to a kernel?

For now, they can be hardcoded into the kernel which of course limits reusablity. I think it makes sense to add scalar arguments to the CLProgram. But I think we can do it later when we need it.

Is the macro approach really scalable? Why not introduce data type changing adapters? They will never be more expensive as any other operator..

The scalability is indeed limited. We should be careful with data type conversion, because otherwise we could have saved ourselves from having such a great type variety in the first and only support one type of rasters. Either way I think this is independent of the main focus of this PR which is the open cl interface.

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.

3 participants