Conversation
ranlu
left a comment
ranlu
left a comment
There was a problem hiding this comment.
Other than the comments in the code, can you also combine the last 3 fixes into the main change (first commit), to make the history clean.
|
|
||
| def _watershed( | ||
| np.ndarray[np.float32_t, ndim=4] affs, | ||
| np.ndarray[uint64_t, ndim=3] segmentation, |
There was a problem hiding this comment.
Why only specify the types of affs and segmentation, and only in this function?
| aff_threshold_low=0.01, | ||
| aff_threshold_high=0.99, | ||
| size_threshold=50, | ||
| # agglomeration_threshold=0.0, |
There was a problem hiding this comment.
I think watershed function should not do agglomeration, can you remove this line and agglomeration code in other places.
| aff_t aff_threshold_low, | ||
| aff_t aff_threshold_high) { | ||
|
|
||
| std::shared_ptr<affinity_graph<aff_t>> affs_ptr( |
There was a problem hiding this comment.
why not use affinity_graph_ptr<aff_t> added to types.hpp?
| boost::fortran_storage_order() | ||
| ), | ||
| // remove deleter because the obj is actually owned by Python | ||
| [](affinity_graph<aff_t>*){} |
There was a problem hiding this comment.
Is this necessary? The shared point is a pointer to a multi_array_ref, delete the ref should not delete the actual affinity data.
| #include "ws/utils.hpp" | ||
| #include "abiss_wrapper.h" | ||
|
|
||
| std::vector<std::size_t> run_watershed( |
There was a problem hiding this comment.
In general it is a bad idea to rely on other headers to include std stuff for you, if you use std::vector, std::array, you should explicitly include them here or in abiss_wrapper.h
| affs = np.asfortranarray(affs) | ||
|
|
||
| volume_shape = (affs.shape[0], affs.shape[1], affs.shape[2]) # XYZ | ||
| segmentation = np.asfortranarray(np.zeros(volume_shape, dtype=np.uint64)) |
There was a problem hiding this comment.
np.zeros(volume_shape, dtype=np.uint64, order='F') does not work?
| #define ABISS_WRAPPER_H | ||
|
|
||
| using seg_t = uint64_t; | ||
| using aff_t = float; |
There was a problem hiding this comment.
seg_t and aff_t have been defined in global_types.h, either include that file or use a different alias for the python code to avoid collision/confusion.
| include_dirs=include_dirs, | ||
| language='c++', | ||
| # extra_link_args=['-lboost_iostreams', '-fopenmp', '-ltbb'], # only necessary for when agg is included | ||
| extra_compile_args=['-std=c++17', '-w'], |
There was a problem hiding this comment.
I would like to keep c++20, to be consistent with the rest of the code, and do not turn off warning during compiling, actually you might want to replace '-w' with '-Wall'
| using volume_ref = boost::multi_array_ref<T,3>; | ||
|
|
||
| template < typename T > | ||
| using const_volume_ref = boost::const_multi_array_ref<T,3>; |
There was a problem hiding this comment.
Do you actually use const_* anywhere or plan to use them? otherwise remove them.
fix: release GIL during watershed call
3 participants
No description provided.