Add filter that generates a tilt series of images from an ImageGeom #378
Conversation
imikejackson
added
Enhancement
| static const float epsijk = 1.0f; | ||
| // static const double epsijkd = 1.0; | ||
| #elif DREAM3D_ACTIVE_ROTATION | ||
| static const float epsijk = -1.0f; | ||
| static const double epsijkd = -1.0; |
There was a problem hiding this comment.
Avoid static global variables (not contained in class or function). As of C++17, global constants should be purely const as it implies a single variable for compiler optimization, but what I've seen most commonly agreed upon is that static const global variables are inherently unsafe. The most common understanding I've seen is that each translation unit (.cpp) will have their own version of the variable. Arguably, that's the same as const pre-17, but it looks like it's not, which can be dangerous.
I am not sure if static const globals post-17 break the assumptions for const globals, but there's a number of articles warning to not compare static const global variables with == operator due to differing memory addresses. The pre-17 way to ensure you only have one version of a global is to use everyone's all-time favorite keyword, extern.
This is also a .cpp file, so none of that applies. But we still should not be using static here.
| DataContainerArray::Pointer dca = getDataContainerArray(); | ||
|
|
||
| #ifdef SIMPL_USE_PARALLEL_ALGORITHMS | ||
| tbb::task_scheduler_init init; |
There was a problem hiding this comment.
Can we use ParallelTaskAlgorithm here? Branching code paths is a code smell, and thread count is already checked in its execute as long as the algorithm object is created before the for loop. If SIMPL_USE_PARALLEL_ALGORITHMS is not supported, it is not performed in parallel.
There was a problem hiding this comment.
I don't think we can use ParallelTaskAlgorithm due to how the loops are developed with the rotations. I will continue to look at the code to see if we can shoe horn the algorithm into the ParallelTaskAlgorithm class.
There was a problem hiding this comment.
If you can't use ParallelTaskAlgorithm, how can multi-threading be supported? It's literally operating the same way as what's written in the loop? Surely, they'd both have the same issue?
mmarineBlueQuartz
left a comment
mmarineBlueQuartz
left a comment
There was a problem hiding this comment.
Overall looks good. There were two points of interest, but I am mostly interested in the multi-threading part due to branching code paths around #ifdef.
updates BlueQuartzSoftware#378 Signed-off-by: Michael Jackson <[email protected]>
updates BlueQuartzSoftware#378 Signed-off-by: Michael Jackson <[email protected]>
imikejackson
force-pushed
the
topic/tilt-series-filter
branch
from
4eecce2 to
2c935a1
Compare
updates BlueQuartzSoftware#378 Signed-off-by: Michael Jackson <[email protected]>
imikejackson
force-pushed
the
topic/tilt-series-filter
branch
from
01a6e57 to
8f23060
Compare
updates BlueQuartzSoftware#378 Signed-off-by: Michael Jackson <[email protected]>
JDuffeyBQ
force-pushed
the
topic/tilt-series-filter
branch
from
8f23060 to
7a09916
Compare
imikejackson
force-pushed
the
topic/tilt-series-filter
branch
2 times, most recently
from
e4abd43 to
24a5830
Compare
A deeper unit test that has exemplar output images needs to be created.
updates BlueQuartzSoftware#378 Signed-off-by: Michael Jackson <[email protected]>
* GenerateTiltSeries::generateGrid() could try to create a grid coordinates array with 0 elements and then access it depending on the user input for the spacing. * Changed the axis constants in GenerateTiltSeries to be static constexpr
…eIds. This allows the developer to more easily debug the rotated geometries complete with the proper data.
Signed-off-by: Michael Jackson <[email protected]>
Signed-off-by: Michael Jackson <[email protected]>
The generic versions were to difficult and obtuse to get working. Signed-off-by: Michael Jackson <[email protected]>
JDuffeyBQ
force-pushed
the
topic/tilt-series-filter
branch
from
24a5830 to
03b85af
Compare
* Compares against GenerateTiltSeriesTest.dream3d from DREAM3D_Data repo
* Ran clang-format
3 participants
Also a fix for a bug in ImageGeom when calculating the index of a voxel from a set of 3D Floating point coordinates.