Review of cpp code. - Githubissues

spherical-volume-rendering / svr-algorithm

A spherical volume rendering algorithm that performs ray casting through a spherical voxel grid.

Other

6 stars 7 forks source link

Review of cpp code. #61

Closed cgyurgyik closed 4 years ago

cgyurgyik commented 4 years ago

@matthewturk I pushed the current PR since we'll be working on it today. As mentioned on Slack, any early failure will pay dividends in the future. Right now, we're still trying to translate our MATLAB version into C++, work out some kinks of the spherical traversal, and verify with testing.

Please take a look at the cpp code, and the Cython code as well, where my knowledge dips. I've been relying solely on Cython documentation for now.

matthewturk commented 4 years ago

Hey, so I started with the cython code (it was shorter!) and I had a few comments.

Usually in yt we specify the precision exactly, so instead of double use np.float64_t for the input into walk_spherical_volume, although maybe that's not necessary since you use double in the C++ code. I guess just an assert that sizeof is the same would suffice?
Line length, which is a simple thing.

Otherwise I think it looks good; are there any allocations that need to be handled?

cgyurgyik commented 4 years ago

Hey, so I started with the cython code (it was shorter!) and I had a few comments.

Usually in yt we specify the precision exactly, so instead of double use np.float64_t for the input into walk_spherical_volume, although maybe that's not necessary since you use double in the C++ code. I guess just an assert that sizeof is the same would suffice?

Line length, which is a simple thing.

Otherwise I think it looks good; are there any allocations that need to be handled?

Ok I addressed these in #63. As far as allocations, not sure yet. Right now I simply allocate everything on the stack, but when we start integrating into yt this may change.