Add configurable binning origin for texture features#328
Add configurable binning origin for texture features#328walkerbdev wants to merge 1 commit intoPolusAI:mainfrom
Conversation
walkerbdev
force-pushed
the
bin_origin
branch
2 times, most recently
from
7330ee1 to
74e4fd2
Compare
vjaganat90
left a comment
vjaganat90
left a comment
There was a problem hiding this comment.
A few fixes and then it is good.
src/nyx/features/texture_feature.h
Outdated
| // matlab binning (BinningOrigin::zero) | ||
| auto n = I.size(); | ||
| prep_bin_array_matlab (max_I_inten, n_levels); | ||
| for (size_t i = 0; i < n; i++) |
There was a problem hiding this comment.
Use for_each or range-based for if order evaluation is not important.
src/nyx/features/texture_feature.h
Outdated
| // matlab binning (BinningOrigin::zero) | ||
| auto n = I.size(); | ||
| prep_bin_array_matlab(max_I_inten, n_levels); | ||
| for (size_t i = 0; i < n; i++) |
src/nyx/python/new_bindings_py.cpp
Outdated
| int neighbor_distance = -1, | ||
| float pixels_per_micron = -1, | ||
| uint32_t coarse_gray_depth = 0, | ||
| int coarse_gray_depth = 0, |
There was a problem hiding this comment.
why change from unsigned to signed int?
| int ram_limit_mb = -1, | ||
| int verb_level = 0) | ||
| int verb_level = 0, | ||
| const std::string& binning_origin = "") |
There was a problem hiding this comment.
If you are using default values, I recommend using by copy semantics, const std::string. Move semantics make copy-ing zero-cost.
If it has to be a reference I suggest universal reference const std::string&&, but try simple value semantics.
| ASSERT_EQ(result, 6); | ||
| } | ||
|
|
||
| // "min" origin, PyRadiomics-style: |
There was a problem hiding this comment.
some of these redundant and noisy comments should be cleaned up.
| s[(int)NyxSetting::SINGLEROI].bval = false; | ||
| s[(int)NyxSetting::GREYDEPTH].ival = -20; // intentionally negative to activate radiomics binCount-based grey-binning | ||
| s[(int)NyxSetting::GREYDEPTH].ival = 20; | ||
| s[(int)NyxSetting::BINNING_ORIGIN].ival = static_cast<int>(BinningOrigin::min_based); |
There was a problem hiding this comment.
we should use consistent casting, either use C-style (int) or C++-safe static_cast at the same place, not mix-match, I would suggest, convert all these (even where your changes don't touch) to consistent static_cast
src/nyx/environment.cpp
Outdated
| } | ||
|
|
||
| void Environment::set_coarse_gray_depth(unsigned int new_depth) | ||
| void Environment::set_coarse_gray_depth(int new_depth) |
There was a problem hiding this comment.
again why are we changing the type of depth here? are these unsigned -> signed changes justified/documented?
2 participants
Add binning_origin parameter to control where the intensity binning range starts for texture features: "zero" (default, bins from [0, max], Nyxus/MATLAB behavior) or "min" (bins from [min, max], PyRadiomics-compatible)