py-sphviewer icon indicating copy to clipboard operation
py-sphviewer copied to clipboard

Published 20 hours ago • verified publisher icon alejandrobll

Added kernel improvements and refactored files to be consistent

Open JBorrow opened this issue 6 years ago • 5 comments

There were two copies of the kernel that, thankfully, were the same but were being re-defined in multiple places. This MR centralises the kernel definitions in a header (kind of sketchy, but better than the code re-use before and it avoids issues with building shared libraries within python).

My editor also auto-formatted the files I changed, let me know if you would like those specific changes reversed.

JBorrow avatar Oct 02 '19 21:10 JBorrow

c_code.c wasn't used in the code, so that was safe. This means only one kernel was actually used. The file should be removed. Could you please revert the format changed to the code so that I can easily see what has actually changed? The same for your other requests. This doesn't mean that I am unhappy with considering improvement to the style, but this should be independent of the other changes, please.

alejandrobll avatar Oct 07 '19 11:10 alejandrobll

I have reverted the changes in rendermodule.c but not in c_code.c as that's un-used anyway.

Note that here I've removed extra_code.c and re-wrote the contents of that entirely in sph_kernel.h.

JBorrow avatar Oct 07 '19 18:10 JBorrow

Is there anything I can do to help this and the other PRs along?

JBorrow avatar Oct 29 '19 11:10 JBorrow

Again, sorry for taking so long to review this. Thanks for this! I like to have a separate file to put all the kernels.

I do not understand, however, what are the kernel improvements here. Do you mean code structure?

From a kernel point of view, the default kernel hasn't changed. I don't think we should include the kernel number 4 as an option, as it is a weird combination of a 2D kernel that uses 3D smoothing lengths, and the two 3D piece-wise kernels shouldn't be used for making images unless properly integrated. I guess you copied them from extra_code.c?

So, out of 4 kernels, only the default one should stay in the file, unless you convince me of the opposite. Please let me know if I am missing something here.

I will include your suggested change of structure but I will leave only one kernel in that file.

alejandrobll avatar Nov 07 '19 13:11 alejandrobll

I agree; I only kept the other kernels as they were already present in the file. Would you like me to remove them?

JBorrow avatar Nov 08 '19 09:11 JBorrow