arrayfire / arrayfire-dotnet

.NET wrapper for ArrayFire
BSD 3-Clause "New" or "Revised" License
78 stars 21 forks source link

Wrapper Draft #2

Closed royalstream closed 8 years ago

royalstream commented 8 years ago

Greetings,

This is the wrapper draft I promised as per this conversation:

https://groups.google.com/forum/#!topic/arrayfire-users/1N0qYpZFC-Q

It's not completely done but its good enough for review.

Best Regards,

Steven Burns

shehzan10 commented 8 years ago

:+1: Great work!

shehzan10 commented 8 years ago

Once again, fantastic work! Very well and cleanly written. While I was able to build this straight away with VS2015 (again :+1: ), I ran into errors with VS2013 as below

1>D:\workspace\arrayfire-dotnet\ArrayFire\Arith.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Arith.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Array.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Array.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Internal\GCMan.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Internal\GCMan.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Matrix.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Matrix.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Data.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Data.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Util.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Util.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct
1>D:\workspace\arrayfire-dotnet\ArrayFire\Vector.cs(37,7,37,13): error CS1041: Identifier expected; 'static' is a keyword
1>D:\workspace\arrayfire-dotnet\ArrayFire\Vector.cs(37,14,37,23): error CS1518: Expected class, delegate, enum, interface, or struct

We'll target 2013 as the minimum version, as we do with the ArrayFire library.

royalstream commented 8 years ago

Thanks! I appreciate it!

The only thing VS2013 doesn't support is "using static" which is really easy to remove. I can take care of it in no time.

shehzan10 commented 8 years ago

I went through the structure of the sln/proj files. Lets structure it this way: ArrayFire.sln (in the root directory along with readme) | |->Wrapper/ArrayFire.csproj |->Examples/CSharp/helloworld/helloworld.csproj |->Examples/FSharp/helloworld/helloworld.fsproj |->Tests/ (future work. not required now)

What I'm essentially asking is to have a single sln file. That way it is clear to users about which solution file they must open. With the examples, I would ideally want to replicate the arrayfire examples directory structure as closely as possible. So lets start with helloworld directory for now.

shehzan10 commented 8 years ago

As a side note, the d82ae2db19ce8be23c5ff4f18dddecdf16b80874 commit, although is in your name, does not show up under your Github account as a contribution because you used a different name/email combination. And when its such an extensive commit, we want it to be credited to you. You can do one of 2 things: 1) Add the email in that commit to your github account. 2) Use git commit --amend --author "Steven <email@address.com>" and then git push -f

royalstream commented 8 years ago

I just made the project VS2013 compatible and moved some files to match the structure you want.

royalstream commented 8 years ago

Now I'm gonna see if I can amend the authors email like you suggested.

shehzan10 commented 8 years ago

You have more than one commit to change now. I would just recommend adding that hotmail email to your github account. That should automatically let github detech its you. https://help.github.com/articles/adding-an-email-address-to-your-github-account/

shehzan10 commented 8 years ago

Structure is better now. Its working with VS2013 too.

I think you missed making the examples from Examples/HelloCSharp -> Examples/CSharp/helloworld/helloworld.csproj (and other files). I also wouldnt mind making it Examples/helloworld/(both CS and FS projects/src files here) if you think that is more intuitive to users.

royalstream commented 8 years ago

My github account is registered with that hotmail account. I just updated all my commits to use this email as well. Thanks for the heads-up, most of my expertise has been around closed-source projects. Please let me know if further changes are required.

I'm going to change the Examples now to follow the desired structure.

royalstream commented 8 years ago

Do you have any preferences regarding the location of the output files? (bin folder)

shehzan10 commented 8 years ago

The commits look good now!

I would say Wrapper/bin should get the wrapper dll. For examples, a bin directory with each project should get the executables.

shehzan10 commented 8 years ago

you can join our gitter chat to talk about more things.

royalstream commented 8 years ago

Excellent, thanks! I will be back in a short while, let me know if further changes are required.

pavanky commented 8 years ago

One concern I have with this wrapper is that it enforces a limit that arrayfire does not have. The types of the arrays are checked at runtime and converted if necessary.

For example in the C/C++ version of arrayfire, we can add an array of type float and int and it will automatically work. In here, we are limited to adding types of the same type. Enumerating all possible combinations is going to bloat up the code.

royalstream commented 8 years ago

Well I can change the code to get rid of the generics. Array can simply become AFArray. At some point I had it working like that so the change is somewhat simple.

Should I create this a separate branch or should I just go ahead and do it on top of this one?

pavanky commented 8 years ago

@royalstream Any reason to call it AFArray ? considering that the array is already inside ArrayFire namespace ?

royalstream commented 8 years ago

I considered AFArray to avoid having to specify the ArrayFire namespace every single time (otherwise C# complains about ambiguous references) On the other hand, the Array type is rarely referenced directly, Arrays get created using the static methods from the Data class. I will keep it as Array then.

pavanky commented 8 years ago

Can't we do something like this in user code to achieve the same thing?

using AFArray = Arrayfire.Array
royalstream commented 8 years ago

Yes of course. I will commit my changes later today (no generics)

pavanky commented 8 years ago

@royalstream thanks!

royalstream commented 8 years ago

Hi, the Array type without generics is ready. I'm doing some basic testing. I have access to the main repo but I'm unsure about what the next step should be. Should I push my changes to a develop branch and do a PR afterwards?

shehzan10 commented 8 years ago

You can continue to push it to your repository. Once the folks are satisfied, we'll pull it into the master branch.

Since we are close enough to a 3.2 release with unified backend (I saw the comment in on of the files) you can download the installer here and start working on that. It'll be great to have this wrapper working with all 3 backends through the unified backend.

royalstream commented 8 years ago

Thanks! I've been looking forward to it (using the unified backend).

shehzan10 commented 8 years ago

Just as a blanket warning that comes with the installer, the unified backend is still in development and not a release yet. So things can change if necessary.

royalstream commented 8 years ago

I just did the changes to support the unified backend and I'm testing them right now. For some reason all backends work except CUDA, I get an error from ArrayFire when I call af_set_backend: AF_ERR_LOAD_LIB.

That said, your C++ example (unified) works perfectly well with all Backends including CUDA so the CUDA 7.5 installation on my computer seems to be fine.

Any advice or suggestions will be greatly appreciated.

shehzan10 commented 8 years ago

I'm assuming its missing NVVM dlls. You should add the following to post build event

echo copy "$(CUDA_PATH)\nvvm\bin\nvvm64*.dll" "$(OutDir)"
copy "$(CUDA_PATH)\nvvm\bin\nvvm64*.dll" "$(OutDir)"
if errorlevel 1 (
    echo "CUDA NVVM DLLs copy failed due to missing files."
    exit /B 0
)
royalstream commented 8 years ago

I forgot to mention I glanced at the code in symbol_manager.cpp so I set AF_SHOW_LOAD_PATH=1 You C++ example prints all three backends loading, mine only two. I'm going to try with the NVVM dlls

royalstream commented 8 years ago

Thanks! that did it, although I had to change $(OutDir) to $(TargetDir) for some reason with the C# project. I will test a little more and upload my changes.

royalstream commented 8 years ago

Ok, the changes are in. Everything seems to be working fine except deallocation with OpenCL. The test runs fine but when the program is closing it shows an error. The error goes away If I comment my call to af_release_array so it's definitely about that. I'm not allocating that many objects so I will add some print statements and try to figure it out.

royalstream commented 8 years ago

UPDATE The OPENCL error only happens when the program closes. I can do whatever I want (allocate, operations, deallocate) and everything works perfectly until the program has to be closed. It also seems to happen only when I run in DEBUG mode. I'm testing targeting the afopencl.dll directly (as opposed to af.dll) and I have a one line program that simply calls af_info() and I get the error when the program closes so I'm tempted to think .NET is causing the error when unloading the OPENCL DLL.

I've been testing with Mono on my Mac and I have no errors there so it could be related to my PC as well (nVidia doesn't have the best OpenCL support out there).

Does it happen to you? Ideas welcome!

9prady9 commented 8 years ago

@royalstream We have seen this problem earlier and it is Windows only bug that has been observed with specific hardware vendors, more specifically with Intel drivers. Mostly, AMD & NVIDIA devices runs fine with OpenCL backend. Which device are you testing it on ?

royalstream commented 8 years ago

I'm using a GeForce GTX 960 with the GeForce 353.90 Driver. I haven't updated to the latest 358.50. I will do so tonight.

pavanky commented 8 years ago

@royalstream Can you make sure the default OpenCL device being picked is NVIDIA and not Intel ? If you also have an Intel GPU on your machine, it can come up ahead of the nvidia gpu in the list of supported devices.

shehzan10 commented 8 years ago

@pavanky the default OpenCL device mechanism is controlled by the library rather than the wrapper. On my system, I get Intel Graphics -> NVIDIA GPU -> Intel CPU. This is because the OpenCL device priority in the library is GPU->Accelorators->CPU. So which one comes up first is not under the control of the wrapper.

pavanky commented 8 years ago

@shehzan10 I meant that @royalstream may be missing that intel is the default device since he only mentioned nvidia. I know that the wrapper does not control the order.

royalstream commented 8 years ago

@pavanky is correct. It was picking the INTEL device for OPENCL which means @9prady9 answer was spot on. I got so absorbed in tracing the allocations/deallocations that I overlooked that.

shehzan10 commented 8 years ago

@royalstream We are considering merging this PR in. Let us know if you want us to hold out for any additional work needed. For future work you can open new PRs.

royalstream commented 8 years ago

Hi, if you could give me one or two days it would be great. I was waiting for your feedback so I had this on stand-by. There are a couple methods I would like to add if possible.

shehzan10 commented 8 years ago

Sure go ahead. I'll wait for your word.

royalstream commented 8 years ago

I do have one question that arose while implementing the call to af_iota I've been assuming that every time I saw a pair like *const unsigned ndims, const dim_t const dims it meant the dims array had to be (at least) of length ndims. However afiota requires this array to have at least 4 elements (regardless of ndims**). I confirmed that by looking at \src\api\c\data.cpp_ It's an easy change on my side to pass arrays that are always of length 4 (like your C++ dim4 type) but I would like to hear your thoughts on the subject before I make the change.

Thanks!

shehzan10 commented 8 years ago

1) Good catch. I'll go ahead and fix it in ArrayFire.

2) A dim4 object will ALWAYS use a 4-value array. ndims is simply a value to point to the number of non-singleton dimensions in the array. When converting from dim4-> array, the ndims is simply a way to reduce the number of iterations. However, as you pointed out, in the C-api, we can't assume that it's always coming from a dim4, so we can't take it for granted. Hence the fix.

royalstream commented 8 years ago

@shehzan10 I added some additional functionality, namely Diagonal, Upper, Lower, Iota, Constant, Range and Identity Arrays plus WriteArray methods to change the Array's contents. There's of course missing functionality that still could be added (especially regarding vector algorithms) but I guess could still help and create additional PRs after you merge it if that's ok with everybody.

shehzan10 commented 8 years ago

I think that should be fine. We can continue to add more functions. Once we have parity with 3.2, we can tag it.

@pavanky do you want to wait for parity or merge it now?

pavanky commented 8 years ago

@shehzan10 go ahead

shehzan10 commented 8 years ago

Merged into devel and setting devel as the default branch until parity with v3.2