feat: add Kernighan-Lee graph partitioning #9
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Brent-Edwards-job-app"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
As part of my job application for CleverThis, this is code using the Aparapi API.
It implements the Kernighan-Lee algorithm (see https://en.wikipedia.org/wiki/Kernighan%E2%80%93Lin_algorithm ) but uses Aparapi to parallelize two operations.
assigned to @chipuni
Why did you make this class package-private rather than public?
You have been using final on your classes, any particularl reason your test classes arent final?
Any particular reason for the modified hungarian notation as opposed to the java style standard?
The class level javadocs could be a bit more elaborate, but being internal to examples not super critical here, just a comment.
Could use more information (as many of the javadocs could). Namely 1) Can null be returned? 2) Can this (or other methods) throw exceptions, if so that should be listed. Many other javadoc elements are left out that are less critical but may have been helpful, such as @since
Any particular reason you choose to make this a getter (and some other methods not).. how did you pick when to make something a getter (get*) vs not?
Why are these two varibles package-private and not public?
Is this just segmented out for readability.
any reason these variables are package-private
This isnt thread safe and documentation doesnt state as such. We can make it safe by instantiating it in a static block.
Why are we using an assert here rather than throwing an exception? What would be the proper type of exception, and would it be checked or runtime, and why?
Methods arent checked for validity. Are nulls allowed for example?
No indication in comments about validity of arguments (what is the range allowed, when is null allowed, etc).
Name is confusing as is javadocs.
why are you using an array here as opposed to an ArrayList?
Some of my initial concerns, i may add more.
This class is only used within these tests. I don't anticipate other tests needing it. It should have the lowest permission that allows everything to run.
No. These tests probably should be final as well -- I don't anticipate needing to extend them.
We talked about this; it has been several years since I have read the Java Style standard, so I switched to my default (based on Hungarian notation.)
I will rename my variables and classes based on https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html
Roger.
I have vastly updated the JavaDocs.
For me, a getter is a very simple property.
A method that does not start with get represents something that is computed.
These constants should only be used by KRKernelHelper. The way to run the methods is through KrKernelHelper.executeCostDifference and KrKernelHelper.executeMaximalDifference.
Yes.
Yes. I had problems with setting variables within the kernel (especially setting variables that were not in an array), so I had all setting done by KrKernelHelper.
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
added 2 commits
d8a2678b- style: update code to match Oracle's Cava style03ae0e43- style: update even more code.Compare with previous version
Thank you. I have done so.
I have fixed this. Thank you very much.
I have added checking for validity throughout the code.
I have added checks within the code for the validity of these
values (and many others). I have also updated the documentation.
I have updated the name and the javadocs.
I am using an array here because the only way that I found to send data from the GPU was through the GPU writing arrays and the CPU reading it.
I would love to learn other ways to communicate between the GPU and the CPU, but I didn't have time to find them.
Thank you very much for your comments.
I have made extensive documentation to the code, updated many names, and cleaned up the code extensively.
We have a difference of opinion in the code review. As far as I can tell, the code does run on the GPU. When I run KrKernelHelper lines 157 and 170, it reports:
The kernel for executeCostDifference was run under: Intel <GPU>
The kernel for executeMaximalDifferences was run under: Intel<GPU>
This might be because I am using an Intel GPU and I am using OpenCL 3.0.
Thank you so much for your extensive examination of the code.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.