Repository navigation
Fix nine latent defects - #60
Merged
Merged
Conversation
daoCalcPeakShape() computed the y offset of the Gaussian within the ROI
from fit_data->xoff:
yo = -(peak->params[YCENTER] - (double)peak->yi) - fit_data->xoff;
daoInitialize() assigns both offsets 0.5*roi_size - 1.0, in both of its
branches, so xoff == yoff identically and nothing observable changes
today. That is a property of this one code path rather than of the
codebase: spliner/cubic_fit.c derives xoff from sx and yoff from sy,
which differ for a non-square spline.
Working through it, the fixed line gives yo = yi - Y for any yoff, where
Y is the position in the image, while the original leaves an extra
(yoff - xoff) behind. Built the library four ways and fit a single
emitter at x = 25.3, y = 13.7 to confirm:
recovered x recovered y
original, yoff == xoff 25.3000 13.7000
fixed, yoff == xoff 25.3000 13.7000
original, yoff = xoff + 1 25.3000 14.7000
fixed, yoff = xoff + 1 25.3000 13.7000
The fix changes nothing as the code stands and makes the y position
independent of the offset convention, which is what the x line already
does. The perturbed yoff was a temporary local patch for the measurement
and is not part of this commit.
No regression test. With xoff == yoff there is no input that tells the
two versions apart, and nothing in the Python API can set them to
different values. A test here could only pin the coincidence that makes
the bug invisible.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mFitGetPeakPropertyDouble() answered a request for "fg_sum_sc" with the
uncorrected sum, the intended call sitting commented out beside it:
else if (!strcmp(what, "fg_sum_sc")){
for(i=0;i<fit_data->nfit;i++){
values[i] = mFitPeakFgSum(fit_data, &fit_data->fit[i]);
// values[i] = mFitPeakFgSumSensitivityCorrected(...);
}
}
and mFitPeakFgSumSensitivityCorrected() was declared in multi_fit.h with
no definition anywhere in the tree.
This is not an open question, it is an unfinished removal. 0fe8987 added
sensitivity correction; a1520d6 took it out again, saying "Remove
'sensitivity_correction' parameter, decided this option wasn't worth the
complexity", and left the branch and the declaration behind. Delete both.
Note that the property was already unreachable from Python. The
peak_properties dict in dao_fit_c.py lists fg_sum but not fg_sum_sc, and
getPeakProperty() raises MultiFitterException for anything not in it. A C
caller now falls through to the function's own else clause, which prints
"Unrecognized parameter 'fg_sum_sc'!" and leaves the values alone, rather
than quietly answering a different question.
No regression test. The branch was unreachable from Python, so nothing
can observe its removal, and a test asserting that getPeakProperty()
rejects the name would pass on the parent commit too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
peakMask() ended both of its stop slices at -1:
peak_mask[:,parameters.getAttr("x_stop")+margin:-1] = 0.0
peak_mask[parameters.getAttr("y_stop")+margin:-1,:] = 0.0
which stops one element short, so the final column and the final row kept
the value 1.0 rather than being masked out. The two start slices, and the
circular AOI branch above them, are all written correctly.
Nothing changes in practice. 'shape' here is the padded image shape and
the stops are already offset by 'margin', so the row and column in
question are padding, and the maxima finder excludes them independently
through its own margin. The full test suite is unchanged either way. The
slice was still wrong, and it is the kind of wrong that only stays
harmless while two unrelated margins happen to agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spline2D.f() takes (y, x), and so does CSpline2D.py_f(), but it passed
them on the other way round:
def py_f(self, y, x):
return self.py_spline.f(x, y)
so it evaluated the transposed point. CSpline3D.py_f() hands (z, y, x)
straight through and has always been right.
py_f() exists to evaluate in Python the same point that f() evaluates in
C, which is how you check one implementation against the other. A PSF
spline is not symmetric in x and y, so the two answers genuinely differ:
on test_spliner_psf_2d.spline at y = 5.0, x = 6.5, f() returns 0.003209
and py_f() returned 0.002391.
Nothing calls py_f(), which is why this has never mattered. It is also
why it is worth fixing: the one place it would be used is validating the
C library, and there it would report a discrepancy that is not there.
The new tests make py_f() do the job it was written for, comparing it
against f() over a thousand random points. The 2D test fails on the
parent commit; the 3D one is the control and passes on both. With the
fix, py_f() and f() agree to 3.5e-18.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DaxReader gives the frame dimensions a documented fallback, 256x256 with
a printed warning, but two other things it reads from the .inf file had
none, and neither did the movie itself:
complete .inf and .dax ok
.inf with no frame count AttributeError: 'DaxReader' object has
no attribute 'number_frames'
.inf with no endian line AttributeError: ... no attribute
'bigendian', from inside loadAFrame()
.dax missing constructs, then AttributeError:
'NoneType' object has no attribute 'seek'
None of these put correctness at stake, but a truncated .inf or a moved
.dax surfaced as an AttributeError deep in the call stack rather than a
message naming the file. The missing movie was the worst of the three:
the warning was gated behind 'verbose', so by default the reader
constructed silently into an unusable state.
Initialize bigendian and number_frames alongside the image size, then:
- endianness defaults to little, with a warning. Everything this
package writes is little endian.
- a missing .dax raises IOError naming the file, which is what
inferReader() already does for an unsupported extension.
- the frame count comes from the file size. A dax file is a bare block
of 16 bit pixels with no header, so this is exact rather than a
guess, and raising above guarantees the file is there to measure.
Note that raising is a behaviour change: code that constructed a reader
for a file that does not exist, and never read from it, now fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
loadCMOSCalibration() says it returns "the CMOS calibration data as a
list [offset, variance, gain, rqe]". The v1 and v2 branches do. The v0
branch returned
return map(numpy.transpose, data)
which in Python 3 is a lazy iterator, not a list. It cannot be indexed,
and it is exhausted by a single pass.
Every caller in the tree tuple unpacks the result, and that works on an
iterator, which is why this has gone unnoticed. It breaks for any caller
that indexes the result or iterates it twice, and only for users on
legacy v0 calibration files, so it would look like a problem with their
data rather than with the reader.
test_cal_v0 already covered the v0 path but only by unpacking, which an
iterator satisfies. It now checks what the doc string actually promises.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three localization read paths disagree about drift correction, and
none of them said so:
getLocalizations(drift_corrected = False) no doc string
getLocalizationsInFrame(drift_corrected = False) no doc string
getLocalizationsInFrameRange(drift_corrected = False) doc string, silent
localizationsIterator(drift_corrected = True) doc string, silent
So switching between "load them all" and "iterate over them" changes
whether dx, dy and dz have been applied, with nothing to warn you. This
is the mechanism behind the marker bug fixed in 0377f48: zCheck() read
corrected z and compared it against a marker written in raw z.
The defaults are not wrong. Each suits its method's primary use: the
getters hand back what is stored, and the iterator is described as the
easiest way to process a whole file, where corrected coordinates are
what you want. So this is a documentation change, not a behaviour one.
State the default and the reason in all four, cross referencing so that
someone reading one sees the other. Also note on tracksIterator() why it
has no such argument: tracks are built from corrected localizations, so
their coordinates are already corrected and the raw values are gone.
test_sa_h5py_20 pins the defaults. It cannot fail on the parent commit,
since nothing behavioural changes here, but it does fail if the defaults
are ever harmonized: flipping getLocalizations() to True fails it. Given
that documentation is the whole of this fix, that test is the only part
of it that resists being undone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
astigmaticPSF.PSF() and PSFIntegral() scaled z by 0.001 before calling
calcSxSy(), which says the incoming z is nanometers. Every other PSF
module in the simulator takes microns: PupilFunctionBase.getPSFs() and
Spline.getPSFs() both document "x, y in pixels and z in microns", and
that is what h5_data['z'] holds.
The parameters settle which side is wrong. calcSxSy() compares z against
wx_params[1] and [2], which are 0.150 and 0.40. As microns that is a
150nm offset and a 400nm defocusing scale, ordinary astigmatism. As
nanometers it would be 0.15nm and 0.4nm, which is meaningless. So the
parameters are microns and the conversion was backwards.
Fed the simulator's own convention, every z landed within a nanometer of
focus:
z (um) with z*0.001 z used directly
-0.40 sx=1.068 sy=1.068 sx=1.700 sy=1.179
-0.20 sx=1.068 sy=1.068 sx=1.329 sy=1.008
0.00 sx=1.068 sy=1.068 sx=1.068 sy=1.068
+0.20 sx=1.068 sy=1.068 sx=1.008 sy=1.329
+0.40 sx=1.068 sy=1.068 sx=1.179 sy=1.700
A round, z-independent PSF at every z. Nothing calls this module, which
is the only reason it has been dormant: wired into the simulator the way
its siblings are it would silently produce data with no astigmatism and
no z dependence, and fail nothing. That is exactly the degenerate case
that made the first Z fitter measurement in this audit meaningless,
round fixed-width spots that a Z model test cannot tell from correct
output.
Checked that nothing depends on the old convention. No test imports
storm_analysis.diagnostics, the diagnostics scripts build their PSFs
from psf.PupilFunction, psf.GaussianPSF or a spline file, and across the
whole repository the only references to astigmaticPSF are the new test
and one line of simulator/README.txt. calcSxSy() itself is unchanged.
Note that the sibling dhPSF.py has z_min and z_max of -500 and 500,
which are nanometers, so the two hand written PSF modules now disagree
with each other. dhPSF is equally uncalled and is left alone here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The other hand written PSF module had the same unit problem as
astigmaticPSF, in its own idiom. dhPSF has no explicit conversion, but
its range constants
z_min = -500.0
z_max = 500.0
are nanometers, and PSF() interpolates the lobe angle linearly between
them. Fed the simulator's own z in microns, every value sits in the
middle of a range a thousand times too wide, so the angle never moves:
z (um) dx dy angle
-0.50 +1.415 +1.413 45.0
-0.25 +1.415 +1.414 45.0
0.00 +1.414 +1.414 45.0
+0.25 +1.414 +1.415 45.0
+0.50 +1.413 +1.415 45.0
A double helix PSF that does not rotate carries no z information at all,
which is the same degeneracy cc6f128 fixed in astigmaticPSF. Fixing
only one of the pair would have left them disagreeing with each other.
Give z_min and z_max the same physical range in microns, so the lobes
sweep 0 to 90 degrees across it as intended, and say the convention in
the module doc string.
Left alone, both pre-existing: the commented out alternative angle
formula, and the question above it about warning when z falls outside
the range. The range is still unenforced, it is just in the right units
now.
Nothing calls this module either. Across the repository the only
references to dhPSF are the new test and one line of
simulator/README.txt.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nine defects that are incorrect as written but produce correct results
today, each because of a coincidence elsewhere in the code. These are the
ones that turn into real bugs during a later refactor. One commit per
issue, and where a regression test is possible it fails on that commit's
parent.
3acb33f0Use yoff, not xoff, for the y peak shape offsetdaoCalcPeakShape()took the y offset of the Gaussian within the ROI fromfit_data->xoff.daoInitialize()gives both offsets the same value, sonothing changes today, but
spliner/cubic_fit.cderives them fromsxandsyseparately, so equal offsets are a property of one code path ratherthan of the codebase. Verified by building the library four ways: with
yoffperturbed by a pixel the original reports y one pixel out and thefixed version does not.
0847a07dFinish removing the sensitivity corrected foreground sum0fe8987fadded sensitivity correction anda1520d61removed it again,leaving behind an
fg_sum_scbranch that answered with the uncorrectedsum and a declaration for a function with no definition anywhere. Deletes
both. The property was already unreachable from Python.
6302c5e0Mask the last row and column of a rectangular AOIBoth
x_stop/y_stopslices inpeakMask()ended at-1, leaving thefinal row and column unmasked. Harmless because those pixels are padding
that the maxima finder excludes through its own margin.
07112467Stop CSpline2D.py_f from transposing its argumentsCSpline2D.py_f()passed(y, x)on asf(x, y), so it evaluated thetransposed point. Its whole purpose is checking the C implementation
against the Python one, so it would have reported a discrepancy that is not
there. The 3D sibling has always been right.
c9198150Fail clearly on an incomplete .inf or a missing .daxnumber_framesand the endianness had no fallback, and a missing.daxconstructed a reader that then died on a
Nonefile pointer, with thewarning gated behind
verbose. Now: endianness defaults to little, amissing movie raises
IOErrornaming the file, and the frame count comesfrom the file size, which is exact for this format.
Note this is a behaviour change: code that constructed a reader for a file
that does not exist, and never read from it, now fails.
50e2c56aReturn a list from the v0 calibration branchloadCMOSCalibration()promises a list and the v1/v2 branches return one.The v0 branch returned
map(), which is not indexable and is exhausted bya single pass. Every caller tuple unpacks, which works on an iterator,
which is why it went unnoticed.
5919fe60Document the drift_corrected defaultsFour localization read paths, two defaulting to False and one to True, none
of them saying so. This is the mechanism behind the marker bug fixed in
0377f488. The defaults suit each method's primary use, so this statesthem rather than changing them, and adds a test that fails if they are ever
harmonized.
cc6f128b,b98571c7Take z in microns in astigmaticPSF and dhPSFBoth hand written PSF modules expected z in nanometres while everything
else in the simulator, and
h5_data['z'], uses microns. Fed thesimulator's own convention,
astigmaticPSFreturned a round PSF withsx = sy = 1.068 px at every z, and
dhPSFfroze its lobes at 45 degrees.Neither would fail anything; both would silently produce data with no z
information.
Full test suite passes, 277 tests.