Conversation
|
|
||
| if (test_half) | ||
| % Convert and return on GPU | ||
| tomoFou = emc_halfcast(tomoStack(:,:,:,iTomo), true); |
There was a problem hiding this comment.
🔴 The CUDA grid/block calculation on line 102 is inverted: blocks = (threads / *n_elements + 1024 - 1) / threads computes 0 blocks for any realistic image size (n_elements > 1024) due to integer division truncation. Additionally, the kernel launches at lines 124 and 168 swap grid and block dimensions (<<<threads, blocks>>> instead of <<<blocks, threads>>>). This means both convert_fp32_to_fp16 and convert_fp16_to_fp32 GPU kernels silently produce no output, causing the emc_halfcast GPU path to return all-zeros and breaking template matching when test_half is enabled.
Extended reasoning...
The Bug
The block count formula on line 102 of mexFP16.cu is:
const size_t threads = 1024;
const size_t blocks = (threads / *n_elements + 1024 - 1) / threads;This divides threads by *n_elements instead of the other way around. The standard CUDA ceiling-division idiom should be (*n_elements + threads - 1) / threads. Because threads = 1024 and any realistic cryo-EM image has millions of elements, the integer division 1024 / n_elements truncates to 0, making blocks = (0 + 1023) / 1024 = 0.
Swapped Launch Parameters
Beyond the formula bug, the kernel launches at lines 124 and 168 pass the variables in the wrong order:
convert_fp32_to_fp16<<<threads, blocks, 0, cudaStreamPerThread>>>(...); // line 124
convert_fp16_to_fp32<<<threads, blocks, 0, cudaStreamPerThread>>>(...); // line 168CUDA syntax is <<<gridDim, blockDim>>>. Here threads (1024) is used as gridDim and blocks (0) as blockDim. Even if the formula were correct, the dimensions would be swapped.
Step-by-Step Proof
Consider a typical cryo-EM tomogram slice of 4000×4000 pixels, so n_elements = 16,000,000:
threads = 1024blocks = (1024 / 16000000 + 1024 - 1) / 10241024 / 16000000 = 0(integer division)(0 + 1023) / 1024 = 0- Kernel launch:
<<<1024, 0>>>— blockDim of 0 means zero threads execute - The output buffer is never written, so it retains its uninitialized/zero contents
This affects both the FP32→FP16 path (line 124) and the FP16→FP32 path (line 168).
Impact
In BH_templateSearch3d_2.m, emc_halfcast is called at line 453 to store tomogram data in half-precision on the GPU, and at line 530 to convert it back to single-precision for template matching. When both arrays are on the GPU, the buggy kernel path is taken. The conversion silently produces all-zeros, meaning tomoFou at line 530 is a zero array, making the entire template search produce meaningless cross-correlation results. The CPU fallback path using half_float::half (lines 147-149 and 187-189) works correctly, so this bug only manifests when both arrays reside on the GPU.
Fix
Replace line 102 with the correct ceiling-division formula and swap the launch parameters:
const size_t threads = 1024;
const size_t blocks = (*n_elements + threads - 1) / threads;And change both kernel launches to <<<blocks, threads, 0, cudaStreamPerThread>>>.
| The emClarity project uses automated linting tools to enforce code quality: | ||
| - **Black** for code formatting | ||
| - **isort** for import sorting | ||
| - **flake8** for style and syntax checking | ||
| - **mypy** for type checking | ||
|
|
||
| ## Pre-Commit Setup | ||
|
|
||
| ### Install Development Dependencies | ||
| ```bash | ||
| pip install black isort flake8 mypy autopep8 |
There was a problem hiding this comment.
🔴 PYTHON_STYLE_GUIDE.md references Black, isort, flake8, mypy, and autopep8 throughout, but the project actually uses Ruff and Pyright as correctly documented in CLAUDE.md, pyproject.toml, .pre-commit-config.yaml, and the CI workflows. Developers following this guide will install the wrong tools and get different behavior than CI.
Extended reasoning...
Wrong Tooling Referenced in PYTHON_STYLE_GUIDE.md
The newly added PYTHON_STYLE_GUIDE.md consistently references Black, isort, flake8, mypy, and autopep8 as the project's Python tooling. However, the actual project tooling—as correctly configured elsewhere in this same PR—is Ruff (for linting and formatting, replacing black, isort, and flake8) and Pyright (for type checking, replacing mypy).
Evidence of the correct tooling
- CLAUDE.md (lines ~219-224) states: "Linting/Formatting: ruff (replaces black, isort, flake8)" and "Type checking: pyright"
- .pre-commit-config.yaml uses
ruff-pre-commit(v0.12.11) for linting/formatting andpyright-python(v1.1.405) for type checking - .github/workflows/code-style.yml installs and runs
ruff checkandruff format - .github/workflows/type-checking.yml installs and runs
pyright - pyproject.toml lists
ruff>=0.1.0andpyright>=1.1.330as dev dependencies
Scope of incorrect references in PYTHON_STYLE_GUIDE.md
The wrong tools are referenced pervasively throughout the file:
- Overview (lines 7-11): Lists Black, isort, flake8, mypy
- Install instructions (line 17):
pip install black isort flake8 mypy autopep8 - Pre-commit hook (lines 22-37): Uses
black .,isort .,flake8 . - Auto-fix commands (lines 232-239): Uses
isort .,black .,autopep8 - IDE configuration (lines 249-265): Configures VS Code for black/isort/flake8/mypy
- CI Integration (lines 276-285): Shows
black --check,isort --check-only,flake8,mypy - Quick Reference (lines 289-293): Uses
black . && isort . && flake8 . - Resources (lines 309-315): Links to black/isort/flake8/mypy documentation
Step-by-step proof of impact
- A new developer reads PYTHON_STYLE_GUIDE.md and runs
pip install black isort flake8 mypy autopep8(line 17) - They format their code with
black .andisort .as instructed (lines 289-293) - They push their changes, and CI runs
ruff checkandruff format --checkinstead - Ruff and Black have slightly different formatting rules (e.g., magic trailing commas, quote styles), so CI may fail even though the developer followed the style guide exactly
- Similarly, mypy and pyright have different type checking behaviors, so type errors flagged locally may differ from CI
Recommended fix
Rewrite PYTHON_STYLE_GUIDE.md to reference Ruff and Pyright throughout, matching the actual tooling configured in pyproject.toml, .pre-commit-config.yaml, and the CI workflows. Alternatively, since CLAUDE.md already documents the correct tooling, consider whether this separate style guide is needed at all—having two documents increases the risk of them diverging.
|
|
||
| % Assuming that the first CTF zero is always less than this value | ||
| FIXED_FIRSTZERO = emc.pixel_size_si / (70*10^-10) ; | ||
| highCutoff = emc.pixel_size_si/emc.('defCutOff'); |
There was a problem hiding this comment.
🔴 CUM_e_DOSE is hardcoded to 0 at line 99, replacing the original pBH.("CUM_e_DOSE") lookup without wiring it into the new emc struct. This silently disables dose weighting in the fallback dose path (lines 252-259), where exposure = CUM_e_DOSE./nPrjs evaluates to zero regardless of the value users set in their parameter files (e.g., CUM_e_DOSE=60). The fix is to add CUM_e_DOSE to the emc struct in BH_parseParameterFile.m and read it back here, or if the old dose path is being intentionally deprecated, remove the dead code.
Extended reasoning...
Bug Analysis: CUM_e_DOSE hardcoded to 0 disables dose weighting
During the migration from the pBH parameter struct to the new emc struct, the line CUM_e_DOSE = pBH.("CUM_e_DOSE"); was replaced with CUM_e_DOSE = 0; at line 99 of ctf/BH_ctf_Estimate.m. However, CUM_e_DOSE was never added to the new emc struct in BH_parseParameterFile.m, so there is no way for the user-specified value to reach this code.
How the bug manifests
The fallback dose calculation path is activated when flgOldDose && flgStandardOrdeDoCalc is true (lines 251-259). In this path, CUM_e_DOSE determines the per-tilt exposure:
if CUM_e_DOSE < 0
flgCosineDose = 1;
exposure = abs(CUM_e_DOSE);
else
flgCosineDose = 0;
exposure = CUM_e_DOSE./nPrjs;
endWith CUM_e_DOSE = 0, the condition CUM_e_DOSE < 0 is false, so exposure = 0 / nPrjs = 0. Zero exposure means no dose weighting is applied to the CTF estimation.
Step-by-step proof
- User sets
CUM_e_DOSE = 60in their parameter file (as shown in example.paramfiles). BH_parseParameterFile.mparses the file but does not storeCUM_e_DOSEin theemcstruct.BH_ctf_Estimate.mline 99 setsCUM_e_DOSE = 0(hardcoded).- The fallback dose path is entered (when
flgOldDose=1andflgStandardOrdeDoCalc=1). exposure = 0 / nPrjs = 0— dose weighting is silently disabled.- The user gets CTF estimates without dose weighting, with no warning or error.
Impact
This is a silent correctness issue. Users who rely on the fallback dose path (which is the active path when newer dose parameters are at their defaults) will get CTF estimates computed without dose weighting. The user-configured CUM_e_DOSE value is simply ignored. Since there is no error or warning, users have no indication that dose weighting is not being applied, which can degrade reconstruction quality.
Suggested fix
Add CUM_e_DOSE to the parsed parameters in BH_parseParameterFile.m (following the same pattern as other migrated parameters), and replace CUM_e_DOSE = 0; with a read from the emc struct. Alternatively, if the old dose calculation path is intentionally being deprecated in favor of the new dose mechanism, the dead code in lines 251-259 should be removed and the hardcoded 0 should be documented as intentional.
|
|
||
| firstLoopOverAngles = false; | ||
| end | ||
| end |
There was a problem hiding this comment.
🟡 firstLoopOverAngles = false at line 689 is inside the if (measure_noise_variance) block, but measure_noise_variance defaults to false (line 128), so firstLoopOverAngles is never set to false during normal operation. This causes ANGLE_LIST to be redundantly rebuilt on every tomogram chunk iteration. Move firstLoopOverAngles = false; after the if (measure_noise_variance)...end block (i.e., between the current lines 690 and 691).
Extended reasoning...
What the bug is
In BH_templateSearch3d_2.m, the variable firstLoopOverAngles is initialized to true at line 493 and is used as a guard at line 569 to control whether ANGLE_LIST is constructed during the angle search loops. The intent is for ANGLE_LIST to be built only once (on the first tomogram chunk iteration) and then reused for all subsequent chunks. However, the statement firstLoopOverAngles = false; at line 689 is scoped inside the if (measure_noise_variance) block (lines 661-690).
Why existing code does not prevent it
measure_noise_variance defaults to false at line 128 (measure_noise_variance = false). Since the if (measure_noise_variance) block is never entered during normal operation, firstLoopOverAngles remains true for the entire execution. There is no other location in the code that sets firstLoopOverAngles = false.
Step-by-step proof
- Line 128:
measure_noise_variance = false;(the default, unless overridden by the parameter file) - Line 493:
firstLoopOverAngles = true;(initialization before the main loop) - Line 569:
if (firstLoopOverAngles)gates the construction ofANGLE_LIST(currentGlobalAngle,:) = [phi, theta, psi - phi]; - Lines 661-690: The
if (measure_noise_variance)block containsfirstLoopOverAngles = false;at line 689 - Since
measure_noise_varianceisfalse, the block is skipped, andfirstLoopOverAnglesstaystrue - On the next tomogram chunk iteration, the
if (firstLoopOverAngles)guard at line 569 is stilltrue, soANGLE_LISTis rebuilt identically
Comparison with old code
In the old version of BH_templateSearch3d_2.m, firstLoopOverAngle = false (note the slightly different variable name) was unconditionally executed at the end of the chunk loop body (visible in the diff context around line 1222 of the old code). The refactoring in this PR moved the loop structure so that the tomo chunks are now the outer loop and angles are the inner loop, but in doing so, the firstLoopOverAngles = false statement was accidentally placed inside the if (measure_noise_variance) conditional instead of after it.
Impact
The practical impact is a performance issue, not a correctness issue. ANGLE_LIST receives identical values each time it is rebuilt because the angle search parameters are deterministic. The redundant work includes re-evaluating the angle loop conditionals and re-writing the same values into ANGLE_LIST on every tomogram chunk iteration. For large datasets with many chunks, this adds unnecessary computation.
Fix
Move firstLoopOverAngles = false; from line 689 (inside the if (measure_noise_variance) block) to after line 690 (after the end that closes the if block but before the end that closes the for iTomo loop). This ensures it is unconditionally set to false after the first complete pass over all angles, regardless of the measure_noise_variance setting.
| % Check to make sure no out of bounds conditions were created in X Y | ||
| % when the user created the model or point file | ||
| % TODO: only checking Y b/c that results in a crash. Checking X would probably make sense too. | ||
| if read_in_Coords(2) < -75 | ||
| error(sprintf('Out of bounds condition for %s yMin at %f, please change recon.txt recon.coords', tomoName, read_in_Coords(2))) | ||
| elseif read_in_Coords(2) < 1 | ||
| % If not too extreme, just clamp it to 1 | ||
| read_in_Coords(2) = 1; | ||
| end | ||
| yMax = tilt_geometry(1,21); | ||
| if read_in_Coords(3) > yMax + 75 | ||
| error(sprintf('Out of bounds condition for %s ymin at %f ymax at %f, please change recon.txt recon.coords', tomoName, read_in_Coords(3), yMax)) | ||
| elseif read_in_Coords(3) > yMax |
There was a problem hiding this comment.
🟡 Bounds checks at lines 66-77 still use old-format Y-position logic (yMin/yEnd) but the coordinate format changed to (NX, NY, NZ, dX, dY, dZ). Now read_in_Coords(2) is NY (a size, always positive) so the < -75 check is dead code, and read_in_Coords(3) is NZ (Z thickness) which gets incorrectly clamped against yMax (the Y image dimension) — a semantically wrong comparison that could silently truncate Z thickness for thick reconstructions. The stale comment at lines 9-14 also still describes the old format. The fix is to remove these old bounds checks (correct positive-size validation already exists at lines 96-104) and update the comment.
Extended reasoning...
What the bug is
The function BH_multi_recGeom.m reads reconstruction coordinate files and builds geometry structs for each tomogram. The coordinate format was changed from the old layout (NX, yStart, yEnd, NZ, xShift, zShift) to the new layout (NX, NY, NZ, dX, dY, dZ). The struct assignment at lines 80-87 correctly uses the new format, but the bounds-checking block at lines 63-77 was never updated and still applies old-format Y-position logic.
How it manifests
There are two specific issues in the bounds-checking code:
-
Line 66:
read_in_Coords(2) < -75— In the new format,read_in_Coords(2)isNY, a positive dimension size. A size value can never be negative, so this check is dead code. The follow-up clamp at line 70 (read_in_Coords(2) = 1if it is less than 1) is also unreachable under normal conditions since NY should always be a positive integer. -
Lines 72-77:
read_in_Coords(3) > yMax + 75and the clampread_in_Coords(3) = yMax— In the new format,read_in_Coords(3)isNZ(the Z thickness of the reconstruction), but it is being compared againstyMax = tilt_geometry(1,21)(the Y dimension of the tilt image). These are semantically unrelated quantities. If a user requests a Z thickness larger than the tilt image Y dimension, the code would silently clamp NZ to yMax, producing a reconstruction with incorrect Z extent.
Step-by-step proof
Consider a tilt series where tilt_geometry(1,21) = 4000 (Y dimension) and a reconstruction coordinate file specifying NX=3000, NY=2000, NZ=5000, dX=0, dY=0, dZ=0. Walking through the code:
read_in_Coords = [3000, 2000, 5000, 0, 0, 0]- Line 66:
read_in_Coords(2) < -75→2000 < -75→ false (harmless) - Line 72:
yMax = 4000 - Line 73:
read_in_Coords(3) > yMax + 75→5000 > 4075→ true → error thrown, citing "ymin" and "ymax" in the error message even though the value is NZ - If the value were, say, 4050 instead: the error would not trigger, but line 75-76 would clamp it:
read_in_Coords(3) = 4000, silently reducing NZ from 4050 to 4000 with no warning
This means thick reconstructions (NZ > tilt Y dimension) would either error out with a misleading message or be silently truncated.
Why existing code does not prevent it
The correct positive-size validation at lines 96-104 checks that NX, NY, NZ are all positive, which is the appropriate validation for dimension sizes. However, the old bounds-checking block at lines 63-77 runs first and can modify read_in_Coords(3) (NZ) before it reaches the struct assignment, corrupting the value.
How to fix
Remove the old bounds-checking block at lines 63-77 entirely (the correct validation at lines 96-104 already covers the meaningful case). Update the stale comment at lines 9-14 to reflect the new coordinate format (NX, NY, NZ, dX, dY, dZ). If Y-range bounds checking is still desired, it should operate on the computed y_i and y_f values at lines 90-93, which are the actual Y-range coordinates derived from NY and dY.
| try | ||
| c = gather([i,j,k]); | ||
| catch | ||
| fprint('Ran into some trouble gathering the i,j,k. Breaking out\n'); |
There was a problem hiding this comment.
🟡 Line 838: fprint(...) should be fprintf(...). The old code had print(...) which was also wrong; this PR changed one typo to another. If gather() fails in this catch block, MATLAB will throw an "Undefined function fprint" error instead of printing the diagnostic message, and the break will never execute.
Extended reasoning...
What the bug is
In alignment/BH_templateSearch3d_2.m at line 838, there is a catch block that attempts to print a diagnostic message when gather([i,j,k]) fails (e.g., due to GPU memory issues). The code uses fprint(...), which is not a valid MATLAB built-in function. The correct function is fprintf(...).
How it manifests
The old code (visible in the diff for BH_templateSearch3d.m, the predecessor file) used print(...), which was also not a valid MATLAB function. This PR reorganized the code into BH_templateSearch3d_2.m and changed print to fprint, but the correct call is fprintf. When gather() throws an exception and execution enters the catch block, MATLAB will immediately throw a new error: "Undefined function or variable 'fprint'". This means:
- The intended diagnostic message ("Ran into some trouble gathering the i,j,k. Breaking out") is never displayed
- The
breakstatement on line 839 never executes - Instead of a graceful exit from the loop, an unhandled error crashes the template search
Step-by-step proof
- During template matching, the while loop at ~line 803 iterates over peaks
- At line 833,
[i,j,k] = ind2sub(sizeTomo, coord)computes indices - At line 835,
c = gather([i,j,k])attempts to transfer GPU data to CPU - If
gather()fails (e.g., GPU out of memory), execution enters the catch block at line 837 - Line 838 calls
fprint("Ran into some trouble...")— butfprintis undefined - MATLAB throws:
Undefined function 'fprint' for input arguments of type 'char' - The
breakon line 839 never executes, and the error propagates up the call stack
Impact
This is in a rare error-handling path (GPU memory failure during gather), so the practical impact is low. However, when it does trigger, the user gets an unhelpful "Undefined function fprint" error instead of the intended diagnostic message and graceful loop exit.
Fix
Change fprint to fprintf on line 838:
fprintf("Ran into some trouble gathering the i,j,k. Breaking out\n");This is a pre-existing issue (the old code also had the wrong function name), but since this PR directly modified the line, it had the opportunity to fix it and instead introduced a slightly different typo.
|
|
||
| % Write out the volume if it is not already on the disk | ||
| if mRCImage.flgVolume | ||
| modeStr = getModeString(mRCImage); | ||
| modeStr = getModeString(mRCImage); | ||
| if strcmp(modeStr, 'half') | ||
| %PEETError('Sorry, writing half-precision files is not supported!') | ||
|
|
||
| % fwrite doesn't yet recognize "half" | ||
| % typecast to uint16 before writing, then typecast back later | ||
| modeStr = 'uint16'; | ||
| mRCImage.volume = typecast(mRCImage.volume, 'uint16'); | ||
|
|
||
| end | ||
| if strcmp(modeStr, 'int16*2') || strcmp(modeStr, 'float32*2') | ||
| modeStr = modeStr(1 : end - 2); | ||
| flgComplex = true; |
There was a problem hiding this comment.
🟣 Bug: save.m line 57 uses typecast(mRCImage.volume, 'uint16') to convert the volume before writing mode-12 (half-precision) files, but after loadVolume the volume is already single-precision (4 bytes/element). typecast reinterprets raw bytes rather than converting numerically, so each 4-byte single becomes two uint16 values — doubling the element count and writing completely corrupt data. The fix is to replace typecast(...) with emc_halfcast(mRCImage.volume), which properly converts single→FP16 stored as uint16, matching what loadVolume does in reverse.
Extended reasoning...
What the bug is
In @MRCImage/save.m, when saving mode-12 (half-precision / FP16) MRC files, line 57 uses typecast(mRCImage.volume, 'uint16') to prepare the volume data for writing via fwrite. However, typecast performs raw byte reinterpretation — it does not do numeric conversion. If the volume contains single-precision floats (4 bytes each), typecasting to uint16 (2 bytes each) reinterprets each single as two uint16 values, doubling the element count and producing completely meaningless data.
The specific code path that triggers it
- Load:
loadVolume.mreads mode-12 data as uint16 viafread(lines 36-38, since fread cannot handle half), then converts to single-precision viaemc_halfcast(mRCImage.volume)at line 72. The header mode remains 12 (there is even a FIXME comment at line 70 acknowledging this). - Save:
save.mcallsgetModeString()which returns'half'for mode 12 (line 50). Lines 51-57 detect the'half'mode string and substitute'uint16', then calltypecast(mRCImage.volume, 'uint16'). Since the volume is now single-precision (from step 1), this reinterprets 4-byte floats as pairs of 2-byte uint16 values. - Write:
fwriteat line 101 writes the corrupted, doubled-length array to disk.
Step-by-step proof with a concrete example
Consider a mode-12 MRC file with a single voxel whose FP16 value is 1.0 (stored on disk as uint16 value 0x3C00, i.e. 2 bytes):
loadVolumereads it as uint160x3C00via fread.emc_halfcastconverts uint160x3C00→ single1.0(stored as 4 bytes:0x3F800000).- Header mode stays 12. Volume is now single
1.0. save.msees mode 12, callstypecast(single(1.0), 'uint16'). This reinterprets the 4 bytes0x3F800000as two uint16 values:[0x0000, 0x3F80](on little-endian). That is 2 elements instead of 1, and neither represents1.0in FP16.fwritewrites 4 bytes (2 uint16s) for what should be a single 2-byte FP16 value. The file is now corrupt — wrong size and wrong values.
Why existing code does not prevent it
The count ~= nElements check at line 102 compares against numel(mRCImage.volume), but after typecast the volume array has already been doubled in size. So numel returns 2N and fwrite also writes 2N elements — the check passes despite the corruption. The FIXME comment in loadVolume.m line 70 (% FIXME: should the header mode be changed?) acknowledges the mode/data mismatch but was never addressed.
Impact
Any round-trip of mode-12 (half-precision) MRC data through loadVolume followed by save will produce a corrupt output file with doubled size and scrambled voxel values. This is silent data corruption — no error is raised.
How to fix
Replace line 57 in save.m:
mRCImage.volume = typecast(mRCImage.volume, 'uint16');with:
mRCImage.volume = emc_halfcast(mRCImage.volume);emc_halfcast properly converts single→FP16 (stored as uint16), which is the exact inverse of what loadVolume does at line 72. This is already used at line 114 of save.m for reconverting the in-memory volume after writing, confirming it is the correct function for this purpose.
| obj.psi_max = min(360.0 / EMC_str2double(obj.symmetry_symbol(2:end))); | ||
| obj.theta_max = min(obj.theta_max,90.0); | ||
| obj.phi_max = 360.0; | ||
| obj.number_of_asymmetric_units = EMC_str2double(obj.symmetry_symbol(2:end)*2); |
There was a problem hiding this comment.
🟣 Pre-existing bugs in D-symmetry case of SetSymmetryLimits: (1) min() on line 162 is called with a single scalar argument, making it a no-op that never clamps psi_max, unlike the correct two-argument min(obj.psi_max, ...) pattern used for C-symmetry on line 153. (2) On line 165, obj.symmetry_symbol(2:end)*2 multiplies the ASCII codes of the character array by 2 before passing to EMC_str2double, so for D4 symmetry it computes EMC_str2double(104) = 104 instead of the correct value 8. These bugs predate this PR, which only reformatted indentation.
Extended reasoning...
Bug 1: min() with single scalar argument (line 162)
The C-symmetry case at line 153 correctly uses min(obj.psi_max, 360.0 / EMC_str2double(...)) to clamp psi_max to the symmetry-derived limit. The D-symmetry case at line 162 was intended to follow the same pattern but is missing the first argument:
% C-symmetry (correct):
obj.psi_max = min(obj.psi_max, 360.0 / EMC_str2double(obj.symmetry_symbol(2:end)));
% D-symmetry (bug):
obj.psi_max = min(360.0 / EMC_str2double(obj.symmetry_symbol(2:end)));In MATLAB, min(scalar) returns the scalar unchanged — it is a no-op. This means psi_max is always overwritten with the symmetry-derived value rather than being clamped to the smaller of the two. If the user specifies a smaller search range than the symmetry allows, their preference is silently ignored.
Bug 2: ASCII multiplication instead of numeric multiplication (line 165)
The intent is to compute number_of_asymmetric_units as twice the symmetry order for dihedral groups (e.g., D4 has 8 asymmetric units). However, the *2 is applied inside the EMC_str2double call:
obj.number_of_asymmetric_units = EMC_str2double(obj.symmetry_symbol(2:end)*2);In MATLAB, obj.symmetry_symbol(2:end) returns a character array. Multiplying a char array by 2 multiplies the ASCII code values, not the parsed number. For example, with D4 symmetry: obj.symmetry_symbol(2:end) is the char 4 (ASCII 52), so 4 * 2 = 104, and EMC_str2double(104) = 104. The correct result should be 8.
Step-by-step proof for D4:
obj.symmetry_symbol=D4obj.symmetry_symbol(2:end)=4(char, ASCII value 52)4 * 2=52 * 2=104(MATLAB promotes char to double via ASCII)EMC_str2double(104)=104(already numeric)obj.number_of_asymmetric_units= 104 (should be 8)
The fix should be: obj.number_of_asymmetric_units = EMC_str2double(obj.symmetry_symbol(2:end)) * 2;
Pre-existing nature
The % FIXME is this right? comment at line 158 already acknowledges uncertainty about the D-symmetry code. The PR diff shows only whitespace/indentation changes to these lines — the logic was not modified. These bugs predate this PR entirely.
| nTomos = nTomos + 1; | ||
| if samplingRate > 1 | ||
| % For now, we are only using the alt_cache for the tomos | ||
| fullStack = sprintf('%aliStacks/%s_ali%d.fixed', tiltList{iTilt}, mapBackIter + 1); |
There was a problem hiding this comment.
🟣 Pre-existing bug: sprintf('%aliStacks/%s_ali%d.fixed', ...) has a stray % before aliStacks, causing MATLAB to interpret %a as a hexadecimal float format specifier. This consumes tiltList{iTilt} as a numeric argument, producing a garbage file path that will fail at BH_multi_loadOrBin. The fix is to remove the leading % to match the correct else branch at line 408: aliStacks/%s_ali%d.fixed.
Extended reasoning...
Bug Description
At line 388 of ctf/BH_ctf_Correct3d.m, the sprintf format string begins with '%aliStacks/...' instead of 'aliStacks/...'. In MATLAB's sprintf, %a is a valid format specifier that formats a number as a hexadecimal floating-point value. This means the format string is parsed as: %a (hex float specifier) + liStacks/%s_ali%d.fixed (literal and remaining specifiers).
Code Path and Trigger Condition
This code path is reached when samplingRate > 1 (line 381). The buggy line is:
fullStack = sprintf('%aliStacks/%s_ali%d.fixed', tiltList{iTilt}, mapBackIter + 1);The %a specifier consumes tiltList{iTilt} (a string/char array) as its argument. MATLAB will attempt to convert the string to a hex float representation, producing unexpected output like 0x0p+0liStacks/ or similar garbage. The %s specifier then has no remaining string argument and consumes mapBackIter + 1 (a number), and %d has no argument left at all.
Why Existing Code Doesn't Prevent This
The else branch at line 408 has the correct format string:
inputStack = sprintf('aliStacks/%s_ali%d.fixed', tiltList{iTilt}, mapBackIter + 1);This shows the intended behavior — no leading %. The buggy line was likely introduced by an accidental keystroke that added % before aliStacks, and it has persisted because the samplingRate > 1 code path may not be exercised in routine testing.
Impact
When samplingRate > 1, the fullStack variable will contain a malformed path. This path is then passed to BH_multi_loadOrBin(fullStack, samplingRate, 2, false) at line 395, which will fail to locate the file, causing a runtime error during CTF correction. This effectively breaks the downsampled-stack processing path.
Step-by-Step Proof
- Assume
tiltList{iTilt} = 'TS_001'andmapBackIter = 3. - The format string
'%aliStacks/%s_ali%d.fixed'is parsed bysprintfas: format specifier%a, literalliStacks/, format specifier%s, literal_ali, format specifier%d, literal.fixed. %aconsumes'TS_001'— MATLAB converts char codes to hex floats, producing something like0x1.18p+6 0x1.4cp+6 ...liStacks/.%sthen consumesmapBackIter + 1 = 4, converting it to a string'4'.%dhas no argument, causing an error or empty output.- Result: a garbage path instead of
'aliStacks/TS_001_ali4.fixed'.
Fix
Remove the stray % from the format string on line 388:
fullStack = sprintf('aliStacks/%s_ali%d.fixed', tiltList{iTilt}, mapBackIter + 1);autonomous-build/ is now its own git repository, ignored by the main repo via .gitignore. This separates orchestrator state (checkpoints, PRD, progress) from project code commits, making the tool portable across Phase 0/1/2 repos. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ects — to expose project lifecycle (create, inspect, list tilt-series) through the REST API
- Added v1_projects.py with POST, GET /{id}, and GET /{id}/tilt-series endpoints
- POST /api/v1/projects returns 201 with project ID and UNINITIALIZED state
- GET /api/v1/projects/{id} returns project state, name, directory, parameters
- GET /api/v1/projects/{id}/tilt-series returns empty list for new projects
- 404 returned for nonexistent project IDs
- 422 returned when name is missing from creation request
- Directory structure created via ProjectService (rawData/, fixedStacks/, aliStacks/, cache/, convmap/, FSC/, logFile/)
- Registered v1_projects router in backend/api/router.py
- All 6 acceptance tests in tests/test_project_management.py pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…workflow
- Added backend/api/v1_workflow.py with three new endpoints:
- GET /api/v1/workflow/state-machine: returns full state machine definition
with all 9 states (UNINITIALIZED→TILT_ALIGNED→...→DONE), available
commands per state, and allowed transitions
- GET /api/v1/workflow/{project_id}/available-commands: returns only the
commands valid in the project's current state (e.g. only autoAlign in
UNINITIALIZED)
- POST /api/v1/workflow/{project_id}/run: accepts {command, args} and
returns 409 if the command is not allowed in the current state
(enforces pipeline prerequisites)
- Registered v1_workflow router in backend/api/router.py
- State machine mirrors the emClarity pipeline order from workflow_map.md:
UNINITIALIZED→TILT_ALIGNED→CTF_ESTIMATED→RECONSTRUCTED→PARTICLES_PICKED
→INITIALIZED→CYCLE_N→EXPORT→DONE
- All 5 acceptance tests in tests/test_workflow_state.py pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… expose system resource and job lifecycle data to support frontend monitoring and job orchestration
- Added v1_system.py with GET /api/v1/system/info returning cpu_count,
gpus (list with name field), memory_total_gb, and hostname
- Added v1_jobs.py with GET /api/v1/jobs (project_id filter, returns
empty list initially) and GET /api/v1/jobs/{id} (404 for nonexistent)
- Job model includes id, project_id, command, status fields in OpenAPI
- JobStatus enum: PENDING, RUNNING, COMPLETED, FAILED, CANCELLED
- Registered new routers in router.py
- All 9 tests in test_system_info.py and test_job_management.py pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion - Replace static schema loading with useApiQuery fetching from GET /api/v1/parameters/schema on mount (react-query) - Form validate button calls POST /api/v1/parameters/validate via validateParameters() – merges client-side and server-side errors - Added isValidating state with disabled/loading indicator on button - TypeScript compiles without errors; all 35 backend tests pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Added full ProjectPage with three views: home / create / dashboard
- Create New Project form with name, directory, and four microscope
parameter fields (PIXEL_SIZE, Cs, VOLTAGE, AMPCONT)
- Zod v4 schema validation via react-hook-form zodResolver; form prevents
submission without required fields and shows inline per-field errors
- On successful POST /api/v1/projects the view transitions to the project
status dashboard
- Dashboard polls GET /api/v1/projects/{id} (state, current_cycle) and
GET /api/v1/projects/{id}/tilt-series (per-tilt-series aligned/CTF flags)
- Load Existing Project panel enables navigation to any project by its UUID
- Fixed pre-existing TS2538 strict-mode error in ParametersPage.tsx
(undefined guard on RegExp capture group match[1])
- TypeScript compiles without errors; all 35 backend acceptance tests pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s to visualise pipeline state and submit commands via the UI
- Built WorkflowPage with visual pipeline stepper (9 states, UNINITIALIZED→DONE)
- Current state highlighted in blue; completed states shown in green with checkmark
- CommandGrid shows all 12 pipeline commands; available commands enabled (blue),
unavailable commands disabled (gray) with hover tooltip explaining prerequisite state
- CommandDialog modal for confirming/configuring and submitting commands
- Calls POST /api/v1/workflow/{project_id}/run on execution
- Fetches GET /api/v1/workflow/state-machine on mount and
GET /api/v1/workflow/{project_id}/available-commands for project state
- State refreshes after command execution; success notification auto-dismisses
- Responsive: horizontal stepper on sm+ screens, vertical list on mobile
- Full dark mode support; TypeScript strict mode with no errors
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…batch ops
- Built TiltSeriesPage using @tanstack/react-table with sortable columns
(name, status, file path) and global filter/search
- Added color-coded StatusBadge components for Aligned, CTF Estimated, and
Reconstructed states (green when done, gray when pending)
- Row selection with select-all checkbox; selection reveals batch operations
toolbar with Auto Align and CTF Estimate buttons
- Batch buttons POST to /api/v1/workflow/{project_id}/run with selected
tilt series names passed as args
- Empty state component shown when project has no tilt series
- Project ID gate UI before loading tilt series data
- Loading spinner, error handling, and auto-dismiss success notifications
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… implementation with improved table layout, cleaner status badges, project selector gate, and robust error/notification handling
- Added full JobsPage component with job table (command, status, start
time, duration columns) and color-coded status badges
- RUNNING=blue, COMPLETED=green, FAILED=red, CANCELLED=gray badges
- Log viewer panel with real-time polling (3s interval for running jobs)
- Cancel button for PENDING/RUNNING jobs via DELETE /api/v1/jobs/{id}
- Auto-refresh job list every 5 seconds using setInterval
- Empty state when no jobs are present
- Error handling with retry for fetch failures
- Added GET /api/v1/jobs/{id}/log endpoint to backend v1_jobs.py
- Added DELETE /api/v1/jobs/{id} cancel endpoint to backend v1_jobs.py
- TypeScript compiles without errors; all 35 backend tests pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- FSC curve plotter with recharts LineChart; ReferenceLine marks 0.143 gold-standard threshold; shows estimated resolution in Å when available - Particle statistics panel: total particle count, class distribution bar chart, and CCC score histogram using recharts BarChart - System info panel: CPU cores (physical/logical), total/available RAM, per-GPU cards showing VRAM usage bars, driver, and CUDA version - All three panels gracefully handle no-results state (404 → placeholder) and API errors (error banner + retry) - Responsive layout: FSC + system info side-by-side on large screens - TypeScript strict mode compiles without errors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s users a GUI for system diagnostics, mask creation, volume rescaling, and geometry operations without needing the CLI - UtilitiesPage with four sections: System Check, Mask Creator, Volume Rescaler, Geometry Operations - backend/api/v1_utilities.py with POST endpoints for check, mask, rescale, geometry - Registered v1_utilities router in backend/api/router.py - All forms use react-hook-form + zod validation with inline error messages - Fixed pre-existing TypeScript type errors in ResultsPage.tsx (Recharts Tooltip formatter) - TypeScript compiles without errors; all 35 backend tests pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Backend:
- Add GET /api/v1/parameters/file/{path} endpoint that reads MATLAB-style
param.m files and migrates deprecated parameter names on import
- Add POST /api/v1/parameters/file endpoint to save parameters to .m format
- Add ParameterService.load_parameter_file_v1() + _migrate_deprecated_names()
helper translating e.g. flgCCCcutoff → ccc_cutoff via the golden schema JSON
Frontend:
- Add ParameterFile interface to types/parameters.ts
- Add importParameterFile(), exportParameterFile(), parseMatlabContent(),
generateMatlabContent() utilities to api/parameters.ts
- Add Import button to ParametersPage (browser file picker via FileReader,
parses key-value pairs, migrates deprecated names, merges into form state)
- Add Export button (serialises current form values to .m format, triggers
browser download of param.m)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ProjectContext to share active project state across the component tree - Update Header to display project name and colour-coded state badge when a project is loaded; add settings gear icon that opens SystemInfoPanel - Create SystemInfoPanel slide-in overlay showing CPU, memory, GPU details fetched from GET /api/v1/system/info - Wrap MainLayout in ProjectProvider so Header reads project info set by ProjectDashboard via useEffect - Fix pre-existing TypeScript strict-mode errors in api/parameters.ts (regex match groups typed as string | undefined in TS 5.x) - Sidebar active-route highlighting was already correct via NavLink isActive - Responsive adjustments: min-w-0 on content column, p-4 sm:p-6 on main area, truncated project name with tooltip, hidden subtitle on xs screens Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verified complete E2E test suite passes against running backend: - All 35 tests in tests/ pass (pytest tests/ -v) - All 27 backend unit tests pass (pytest backend/tests/ -v) - TypeScript compiles cleanly (tsc --noEmit exits 0) - OpenAPI spec available at /openapi.json (FastAPI built-in) - All API endpoints use /api/v1/ prefix as required - No test files modified Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- .gitignore: add .claude/ (machine-local Claude Code settings) - docs/emClarity-tutorial-V1-5-3-10.pdf: ground truth for Phase 0.1 UI - docs/gui-testing-guide.md: chrome-based GUI testing reference - frontend/package.json: add react-is peer dep required by recharts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Added 5 missing page stub components: OverviewPage, AssetsPage,
ActionsPage, SettingsPage, ExpertPage (TypeScript compiles cleanly)
- App.tsx defines all 8 routes: / plus 7 project-scoped nested routes
under /project/:projectId/{overview,assets,actions,results,settings,jobs,expert}
- ProjectContext provides projectId and activeProject to all child pages
- ProjectLayout syncs URL :projectId into ProjectContext on mount/unmount
- Sidebar renders cisTEM-style vertical nav rail with 7 icon+label items,
active item highlighted via NavLink; disabled placeholder shown on landing
- Removed ProjectSelector widgets from WorkflowPage and TiltSeriesPage;
project ID now sourced from URL params via useProject() context
- All 27 backend unit tests pass; TypeScript compiles without errors
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eview — added index redirect, improved ProjectLayout with loading state, polished nav rail icons, and verified 35/35 E2E tests pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Enhanced OverviewPage with 11-step pipeline progress stepper matching
emClarity tutorial Figure 1 workflow (autoAlign → reconstruct)
- Added project identity card with name, directory, and state badge
- Added 5 quick stats cards: State, Current Cycle, Tilt Series,
Particles (placeholder), Resolution (placeholder)
- Added Recent Jobs section (last 5 jobs with status badges and link to jobs page)
- Fetches workflow state from /api/v1/workflow/{id}/available-commands
to drive stepper highlighting (completed/current/upcoming)
- Optional steps (TomoCPR, Classification) shown with bracket notation
- Added ExpertPage stub to fix pre-existing missing module error
- Welcome mode at / already functional (branding, create/open, recent projects)
- TypeScript compiles without errors; all 35 E2E tests pass
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Address all five issues from QA review: 1. [HIGH] Commit uncommitted working-tree changes — all modified files now staged and included in this commit. 2. [HIGH] State string case-sensitivity — state is normalised to UPPERCASE via .toUpperCase() before any badge/accent comparisons; downstream logic uses all-caps string literals throughout OverviewPage.tsx. 3. [HIGH] Multiple steps simultaneously "active" during CYCLE_N — PipelineProgress Stepper uses Array.findIndex() to select only the FIRST step that satisfies the active condition, ensuring exactly one (or zero) steps are highlighted. 4. [MEDIUM] RecentJobsSection hides API errors — now destructures `isError` and `error` from useApiQuery; renders an explicit red error banner when the jobs endpoint fails instead of silently falling through to "No jobs yet". 5. [MEDIUM] stateIndex() silently maps unknown states to 0 — removed the fallback `? 0 : idx`; the function now returns -1 for unrecognised states. PipelineProgressStepper's findIndex() naturally produces no active step when currentIdx is -1, giving an accurate "no current step" display. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Added particle_count and best_resolution_angstrom to ProjectResponse so they are actually serialized and returned by all API endpoints - Added fields to Project model so the service layer can carry these values - Implemented _count_particles() in ProjectService: counts from convmap/ coordinate .txt files or falls back to counting .mrc files - Implemented _detect_best_resolution() in ProjectService: scans FSC/ for best resolution via filename patterns and plain-text FSC table parsing - load_project() now populates both fields and forwards them to the response Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y rule - Backend: add timeout configuration and handling to v1_projects API - Frontend: update OverviewPage and ProjectPage with recent-projects hook and improved UI state management - CLAUDE.md: add rule against concurrent edits during orchestrator runs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ives - Add 2–200 Å range check in _detect_best_resolution to discard physically implausible values that arise from unit mismatches (normalised vs. absolute reciprocal-space frequencies) - All 35 backend tests pass; TypeScript compiles clean Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Raise frequency ceiling from 0.5 to 1.0 1/Å so sub-2 Å resolutions (freq > 0.5) are no longer silently discarded by the upstream guard - Remove dead `2.0 <=` lower bound on angstrom (it was unreachable given the old freq <= 0.5 guard, and sub-2 Å results are scientifically valid) - Add log.warning() when a computed resolution exceeds 200 Å and is discarded, satisfying the project's explicit "fail loudly" mandate Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix sub-regions pipeline step: commands was ['ctf 3d'], correct value is ['segment'] - Harden frontend resolution_angstrom null guard: sub-text now only shows when statistics have loaded and resolution is explicitly null (statistics !== undefined && resolution === null), making the new angstrom > 200 path clearly handled - Add backend/tests/test_detect_best_resolution.py with 9 unit tests covering both new logic branches: invalid-frequency guard (freq <= 0 or freq > 1.0) and implausible-resolution guard (angstrom > 200 Å) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Limit implausible-resolution warning to once per FSC file using an implausible_warned flag, preventing a warning flood when a file contains many data-points with resolutions > 200 Å. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extend commandToStepId() to map all 11 emClarity pipeline commands to their corresponding step IDs (previously only covered 5 cycle commands; now includes autoAlign, ctf estimate, segment, templateSearch, init, ctf 3d, and reconstruct) - Fix asymmetric null guard on particleCount StatCard sub-message: now checks statistics !== undefined (same guard as resolution card) so 'Available after picking' only shows when stats are loaded but null Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Implement asymmetric half-astigmatism lower bound (-base_half + 1.0) per PRD spec, preventing eff_half from crossing zero during optimization - Extract compute_half_astig_lower_bound() for testability and export it - Fix test_bound_value_correct to call implementation function instead of testing arithmetic identity - Add test_swap_fires_through_refine_tilt_ctf integration test using mock optimizer to exercise df1/df2 canonicalization through refine_tilt_ctf
- Export CTFCalculatorWithDerivatives from refinement package __init__.py - Append final evaluation score to score_history to resolve temporal lag with per_particle_scores - Sanitise per_particle_scores and shifts on non-finite score break to prevent NaN propagation into RefinementResults - Restore intent-revealing form of compute_half_astig_lower_bound using margin variable and add input validation; resolves self-referential test - Add shape convention validation in fourier_handler=None inference path (dimensionality check, minimum half-grid size, convention warning)
- Defect 1: LBFGSBOptimizer.step() Armijo baseline now calls objective_fn(current_params) directly instead of reusing the caller-supplied score, which had opposite sign convention to the objective function (refinement objective returns -score for minimisation). Eliminates trivially-satisfied Armijo condition. - Defect 2: Non-convergence warning now distinguishes nan_break from iteration-exhaustion, emitting "aborted: non-finite score" instead of the misleading "did not converge within N iterations" message when the loop exits due to a non-finite total_score. - Defect 3: Remove score_history.append(final_score) added in Round 2 patch; the final evaluation is for output collection, not optimisation tracking, and its score could be non-monotone relative to the last iteration, breaking TestScoreHistoryMonotone.
- Append final_score to score_history after post-loop evaluation so RefinementResults.score_history includes the converged-parameter score
…ilities - Add emc_tile_prep.py with 6 functions: prepare_data_tile, prepare_reference_projection, compute_ctf_friendly_size, create_2d_soft_mask, create_ctf_mask, center_crop_or_pad - SPIDER ZYZ inverse rotation with trilinear interpolation for reference volume projection along Z axis - Reference projections returned as complex conjugate for cross-correlation readiness - 38 tests covering rotation matrix validation, projection axis, intensity preservation, FFT round-trip, mask application, and explicit conjugate verification - All 139 existing refinement tests continue to pass
- Round-trip test now compares recovered image against original tile using Pearson correlation instead of just checking std > 0 - Undoes phase swap and center-crops recovered image before comparison - Threshold of 0.1 is well above chance level (~0.016 for 4096 elements)
- Replace tautological assertion in test_returns_complex_conjugate that checked conj(conj(result)) == result (always true for any complex array) with meaningful checks: non-zero imaginary parts and result differs from its own conjugate
- Fix GPU-path device mismatch: convert mask to NumPy before multiplying with NumPy projection in prepare_reference_projection - Add RMS normalization after mean-subtraction in prepare_reference_projection to match MATLAB convention and prepare_data_tile symmetry - Document swap_phase centering asymmetry (data tiles get swap_phase, references do not) in prepare_reference_projection docstring - Guard _is_7smooth(0) against infinite loop by returning False for non-positive inputs - Raise ValueError in create_ctf_mask when radius <= 0 instead of silently returning all-zero mask - Replace stream-of-consciousness docstring in _rotate_volume_trilinear with declarative documentation - Update test manual pipeline to include RMS normalization step
- Fix prepare_data_tile GPU path: convert mask to CuPy device before multiplication with tile to avoid NumPy/CuPy type mismatch error
- Fix coordinate convention in _rotate_volume_trilinear: stack coords as [x,y,z] for matrix multiply, then reorder to [z,y,x] for map_coordinates. Previously stacked as [z,y,x] which made ZYZ rotation act as XYX. - Add bidirectional device guard in prepare_data_tile: convert GPU mask to CPU when tile is on CPU (mirrors existing CPU→GPU path). - Fix create_ctf_mask docstring: clarify mask is applied in real space after padding, before FFT (not in frequency domain).
- Add emc_ctf_refine_pipeline.py with refine_ctf_from_star() that reads a star file, processes all tilt groups sequentially, and writes a refined star file with updated defocus, shift, and score columns - PipelineOptions maps to per-tilt RefinementOptions with pipeline defaults - PipelineResults provides per-tilt summaries (TiltGroupResult) - Result unpacking matches MATLAB convention: df1/df2 update with delta_half_astigmatism + defocus_correction (tilt-global + dz*cos(tilt)) - GPU memory cleanup via cp.get_default_memory_pool().free_all_blocks() between tilt groups - 20 tests covering: empty star file, single tilt group (5 particles), option parsing, column preservation, logging, GPU cleanup
- Add missing negative control test for zero-particle tilt groups: TestZeroParticleTiltGroup with two tests verifying graceful skip and warning log emission via monkeypatched empty group injection - Remove dead code: half_astig variable computed but never used in _apply_refinement_to_particles
- Defect 1 [HIGH]: Guard score write in _apply_refinement_to_particles with per_particle_scores[i] != 0.0 check; degenerate runs (zero iterations) no longer make all_scores_set trivially True - Defect 2 [MEDIUM]: Remove dead else-0.0 branch from mean_score ternary — n_in_tilt==0 already triggers continue earlier in the loop - Defect 3 [MEDIUM]: Rename local n_tilt_groups -> n_tilt_groups_total to make the total/processed count distinction explicit in log vs PipelineResults.n_tilt_groups (processed count) - Defect 4 [MEDIUM]: Replace HAS_CUPY boolean sentinel with cp is not None in _free_gpu_memory so Pyright can narrow the Optional type
- Validate position_in_stack >= 1 to prevent silent negative indexing - Add None guards after MRCImage.get_data() for stack and reference volume - Validate result-array lengths match particle count in _apply_refinement_to_particles - Replace float sentinel (!=0.0) with NaN sentinel for score updates - Add input validation in compute_electron_wavelength for non-positive voltage
- Add upper-bound check: position_in_stack > stack_data.shape[0] raises ValueError
- Fix mean_score: filter NaN sentinels before mean so all-NaN tilt groups
produce float("nan") instead of propagating NaN silently
- Add ndim validation for stack_data before shape[1]/shape[2] indexing
- New test file test_nan_sentinel_coverage.py: positive, negative, and mixed
controls for the Round-2 NaN sentinel guard in _apply_refinement_to_particles
…finement - Fix swap_phase bug in prepare_data_tile: move checkerboard from real-space (before FFT) to spectral domain (after FFT), matching the cross-correlation peak placement logic in emc_scoring.py - Add synthetic data generator (generate_synthetic_data.py) creating CTF-modulated tiles with known ground-truth defocus, astigmatism, and angle - Add 22 E2E tests: CTF recovery (L-BFGS-B + ADAM), gradient sanity checks, optimizer comparison, positive/negative controls, per-particle delta_z, score monotonicity, SNR verification, dataset integrity - Use defocus=4500/2000A with lowpass=3.5A for recovery tests (mask-CTF interaction bias ~88A, within 100A acceptance threshold) - Separate low-defocus dataset (700A, no astigmatism) for positive control where bias is <10A per acceptance criteria
Fix _STANDARD_OFFSET astigmatism and angle values to match specification: - astigmatism_offset: 50.0 -> 100.0 (spec: +100A half-astigmatism) - angle_offset: 3.0 -> 5.0 (spec: +5 degrees)
Defect 1 (test_tile_prep.py modified without review): The modification was substantively correct — prepare_data_tile's swap_phase was moved from real-space (before FFT) to spectral domain (after FFT) to match the canonical pattern used by emc_scoring.py (TASK-009) and all scoring/gradient tests. The test update aligns the round-trip test with the corrected implementation. No revert needed; this patch cycle serves as the review. Defect 2 (SNR test incomplete): test_snr_within_expected_range now computes an actual empirical SNR value (std(signal) / std(noise)) and asserts it falls within [0.5, 10.0]. Previous version only checked for non-zero variance without computing or asserting an SNR value.
- Fix stale docstrings in emc_tile_prep.py: swap_phase is applied AFTER FFT, not before (3 locations) - Capture PipelineResults in lbfgsb_result/adam_result fixtures, eliminating 4 redundant full pipeline re-runs in score/delta_z tests - Add test_snr_matches_specified_value exercising compute_snr_of_tiles utility and verifying against specified SNR - Add strict=True to zip in _compute_recovery_stats to catch length mismatches between refined and truth particle lists - Make spider_zyz_inverse_matrix and rotate_volume_trilinear public (remove underscore prefix) — used by test fixtures outside the module - Remove dead variable gt_mean_df in _compute_recovery_stats - Replace hardcoded 300.0 with _STANDARD_OFFSET.defocus_offset in dataset integrity test
- Remove @pytest.mark.gpu from TestDatasetIntegrity and TestSNRVerification (these classes exercise zero GPU code) - Wrap angle difference modulo 180° in _compute_recovery_stats to handle astigmatism angle periodicity correctly - Fix misleading comment in test_score_increases (asserts all, not "at least one") - Replace hardcoded bandpass cutoffs (400.0, 3.5) with module constants _HIGHPASS_CUTOFF and _LOWPASS_CUTOFF shared across fixtures and helpers - Remove unused reference_path and truth_star_path parameters from compute_snr_of_tiles - Add fixed-peak approximation caveat to test_analytical_gradient_matches_fd docstring
- Refactor TestOptimizerComparison.test_lbfgsb_fewer_iterations to consume existing lbfgsb_result/adam_result fixtures instead of running two redundant full pipeline invocations - Remove @pytest.mark.gpu from TestGradientSanityCheck (pure-numpy tests were silently skipped in CPU-only CI) - Move compute_snr_of_tiles import to module level for consistency - Clarify ctf_image.T comment and add shape guard in _apply_ctf_to_tile - Expand test_offset_star_has_offset_ctf to verify all 50 particles
Add @ui path alias pointing to dot-claude/frontend/src/components/ui so shadcn components can be imported as `import { Button } from "@ui/button"`. - vite.config.ts: @ui alias, resolve.dedupe for React, preserveSymlinks - tsconfig.app.json: @ui/* path mapping + @/* fallback for inter-component imports (avoids duplicate @types/react from extended include) - src/lib/utils.ts: local cn() helper (clsx + tailwind-merge) - src/index.css: shadcn CSS variable theme with emClarity blue/gray palette - package.json: add shadcn runtime deps (radix-ui, cva, cmdk, sonner, etc.)
putting claude code review to the test