cyclops-community / ctf

Cyclops Tensor Framework: parallel arithmetic on multidimensional arrays
Other
194 stars 53 forks source link

simplify python partition interface #128

Closed raghavendrak closed 2 years ago

solomonik commented 2 years ago

@raghavendrak thanks for the changes. It seems the get_idx_partition() function in idx_partition is unnecessary, while for the partition object, I would shorten the function name to idx. Also please add docs in the same style as in tensor.pyx.

solomonik commented 2 years ago

Thanks for the revisions. Two remaining comments: (1) it would make more sense to me if the idx_partition constructor took a partition object and not a lens object as argument, the partition object could always be extracted from an idx_partition object, and an idx_partition argument here is redundant given that idx is another argument, (2) clens needs to be deallocated I think.

solomonik commented 2 years ago

I think still potential for a minor memleak here, the C++ partition objects allocated with new should be deleted. See how this is done in the tensor object, namely

    def __dealloc__(self):
        del self.dt

I am able to check for memory leaks by running stuff like mpirun -np 4 python valgrind which produces some types of garbage errors/warnings, but I find that the actual true errors are distinguishable and can be searched for.