Skip to content

Commit 6a79b63

Browse files
imikejacksonclaude
andcommitted
fix(lib): correct main.cpp voxel ordering + harden simulateMTR per review
FIX 1: Rebuild spatialCoords in SIMPLNX z,y,x order (iz outer, iy middle, ix inner) so row k matches sim.phi*/mtrIndex[k]. The old loop used z-x-y order which mismatched coordinates with orientations for non-square grids. FIX 2: Replace magic literals 72, 36, 72 in the simulateMTR call with named constants (k_OdfBinsPhi1/PHI/Phi2) documenting the fixed 5-degree Bunge-Euler MATLAB ODF grid layout (186624 bins). FIX 3: Guard in simulateMTR that pgrf_result.mtrIndex.size() == N; throws std::runtime_error if the PGRF result dimensions are inconsistent. FIX 4: CSV loop now uses sim.nx*sim.ny*sim.nz to tie the bound to the actual result rather than the pre-call N. FIX 5: Add phi2 range check and phi1/phi/phi2 size-equality checks to the statistical driver test. FIX 6: Remove unused <numbers> include from main.cpp; ODFSampler.hpp retained (ODFComponent is used directly). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f331a9e commit 6a79b63

3 files changed

Lines changed: 31 additions & 14 deletions

File tree

src/LibMTRSim/MTRSimDriver.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,10 @@ MTRSimResult simulateMTR(const SimulationParams& params,
8585
PGRFSimulation pgrf{rng};
8686
const PGRFResult pgrf_result = pgrf.run(params); // throws on bad dims
8787

88+
if (static_cast<int>(pgrf_result.mtrIndex.size()) != N) {
89+
throw std::runtime_error("simulateMTR: PGRF result size does not match grid dimensions");
90+
}
91+
8892
// 2. Sample N orientations per component against the uniform reference.
8993
const ODFComponent uniformOdf = buildUniformODF(n1, nPHI, n2);
9094
const int numComponents = static_cast<int>(odfComponents.size());

src/app/main.cpp

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
#include <cmath>
1616
#include <fstream>
17-
#include <numbers>
1817
#include <random>
1918
#include <stdexcept>
2019
#include <string>
@@ -203,19 +202,20 @@ int main(int argc, char **argv) {
203202
spdlog::info(" dx={} dy={} dz={} [mm]", params.dx, params.dy, params.dz);
204203

205204
// ── Build spatial coordinate matrix
206-
// ────────────────────────────────────────── Ordering matches
207-
// simulate_MTRs.m: z outer → x middle → y inner.
208-
// s(k) = [j*dx, i*dy, zix*dz]
209-
// j ∈ [1,nx], i ∈ [1,ny], zix ∈ [1,nz]
205+
// ──────────────────────────────────────────
206+
// Row order matches simulateMTR's SIMPLNX z,y,x output: z slowest, x fastest.
207+
// k = iz*(ny*nx) + iy*nx + ix
208+
// spatialCoords(k) = [(ix+1)*dx, (iy+1)*dy, (iz+1)*dz]
209+
// 1-based coordinate values are preserved (matching the original convention).
210210
Eigen::MatrixXd spatialCoords(N, 3);
211211
{
212-
int k = 0;
213-
for (int zix = 1; zix <= nz; ++zix) {
214-
for (int j = 1; j <= nx; ++j) {
215-
for (int i = 1; i <= ny; ++i, ++k) {
216-
spatialCoords(k, 0) = j * params.dx;
217-
spatialCoords(k, 1) = i * params.dy;
218-
spatialCoords(k, 2) = zix * params.dz;
212+
for (int iz = 0; iz < nz; ++iz) {
213+
for (int iy = 0; iy < ny; ++iy) {
214+
for (int ix = 0; ix < nx; ++ix) {
215+
const int k = iz * (ny * nx) + iy * nx + ix;
216+
spatialCoords(k, 0) = (ix + 1) * params.dx;
217+
spatialCoords(k, 1) = (iy + 1) * params.dy;
218+
spatialCoords(k, 2) = (iz + 1) * params.dz;
219219
}
220220
}
221221
}
@@ -239,8 +239,14 @@ int main(int argc, char **argv) {
239239

240240
// ── Run full MTR simulation (PGRF + ODF sampling + remap to z,y,x order)
241241
// ─────────────────────────────────────────────────────────────────────────
242+
// The MATLAB ODF HDF5 layout is a fixed 5-degree Bunge-Euler grid:
243+
// 72 (phi1) x 36 (PHI) x 72 (phi2) = 186624 bins.
244+
constexpr int k_OdfBinsPhi1 = 72;
245+
constexpr int k_OdfBinsPHI = 36;
246+
constexpr int k_OdfBinsPhi2 = 72;
247+
242248
spdlog::info("Running MTR simulation...");
243-
mtrsim::MTRSimResult sim = mtrsim::simulateMTR(params, odfComponents, rng, 72, 36, 72);
249+
mtrsim::MTRSimResult sim = mtrsim::simulateMTR(params, odfComponents, rng, k_OdfBinsPhi1, k_OdfBinsPHI, k_OdfBinsPhi2);
244250
spdlog::info("MTR simulation complete.");
245251

246252
Eigen::VectorXd phi1Vec = Eigen::Map<Eigen::VectorXd>(sim.phi1.data(), static_cast<Eigen::Index>(sim.phi1.size()));
@@ -275,7 +281,9 @@ int main(int argc, char **argv) {
275281
csv << std::fixed;
276282
csv.precision(6);
277283

278-
for (int i = 0; i < N; ++i) {
284+
// Use sim dimensions to tie loop bounds to the actual result.
285+
const int simN = sim.nx * sim.ny * sim.nz;
286+
for (int i = 0; i < simN; ++i) {
279287
csv << spatialCoords(i, 0) << ',' << spatialCoords(i, 1) << ','
280288
<< spatialCoords(i, 2) << ',' << phi1Vec[i] << ',' << phiVec[i] << ','
281289
<< phi2Vec[i] << ',' << sim.mtrIndex[static_cast<std::size_t>(i)] << '\n';

tests/test_mtrsim_driver.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,11 @@ TEST_CASE("simulateMTR reproduces target volume fractions (statistical)", "[mtrs
8888
REQUIRE(static_cast<double>(counts[1]) / N == Approx(0.35).margin(0.05));
8989
REQUIRE(static_cast<double>(counts[2]) / N == Approx(0.35).margin(0.05));
9090

91+
REQUIRE(static_cast<int>(r.phi1.size()) == N);
92+
REQUIRE(static_cast<int>(r.phi.size()) == N);
93+
REQUIRE(static_cast<int>(r.phi2.size()) == N);
94+
9195
for (double a : r.phi1) { REQUIRE(a >= 0.0); REQUIRE(a <= 2.0 * std::numbers::pi); }
9296
for (double a : r.phi) { REQUIRE(a >= 0.0); REQUIRE(a <= std::numbers::pi); }
97+
for (double a : r.phi2) { REQUIRE(a >= 0.0); REQUIRE(a <= 2.0 * std::numbers::pi); }
9398
}

0 commit comments

Comments
 (0)