Repository navigation
Remove dead and inconsistent code - #61
Merged
Merged
Conversation
The function is defined and exported, and it is used: Python calls it through ctypes, which resolves the symbol from the shared library and so never consults a header. It was simply the one mFit* function missing from the declaration block. Nothing is broken today, but a C consumer wanting to call it would hit an implicit declaration warning under C99 and a hard error under C23, and the omission makes the header a misleading index of the library's API. Checked by comparing every mFit* definition in multi_fit.c against the declarations in multi_fit.h: with this added, the two agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
multi_fit.h declared mFitIterateOriginal() but nothing defines it. The function was added in fa46abd as an "(untested) iterate() function that duplicates the behavior of the original 3D-DAOSTORM approach", made to work in 93a99b7, and removed by a19f27d on 2018-09-20, "Use circular ROI for fitting", which left the declaration behind. Its parent commit still has the definition. Nothing in the tree has referenced it since, in C or through ctypes. This is the same shape as the mFitPeakFgSumSensitivityCorrected declaration removed in 0847a07, and the other half of what makes the header a poor index of the library: 413d2c1 added the one declaration that was missing, this removes the one that outlived its function. With both, every mFit* definition in multi_fit.c has a declaration in multi_fit.h and every declaration has a definition, 38 each way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HYSTERESIS was defined and never referenced, while mFitUpdate() decided
when to recentre an AOI from two literals:
if((dx<-0.5)||(dx>1.5)){
Reading the constant the way its comment does, as a threshold on how far
a peak may drift from its AOI before the AOI follows, |dx - 0.5| >
HYSTERESIS:
0.5 dx < 0.0 or dx > 1.0 band [0, 1] no hysteresis
0.6 dx < -0.1 or dx > 1.1 band [-0.1, 1.1]
1.0 dx < -0.5 or dx > 1.5 band [-0.5, 1.5] what the code does
So the constant was not documenting a dropped feature. Hysteresis is
live, there is exactly one implementation of it, and every other fitter
reaches it by calling mFitUpdate(). The constant simply carried the
wrong number, 0.6 against an effective 1.0, and nothing tied the two
together.
Set it to 1.0 and write the bounds in terms of it. Taking 0.6 as
authoritative instead would narrow the dead band to [-0.1, 1.1], which
is a real change to peak recentring during fitting and does not belong
in a branch about dead code.
Behaviour preserving, and provably so: multi_fit.o is byte identical
before and after, md5 4b8058df50f0d1b692237950820df4aa. gcc folds
0.5-1.0 and 0.5+1.0 to the same literals the code had.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Its six siblings, dxfAt2D, dyfAt2D, dyfAt3D, dzfAt3D, fAt2D and fAt3D,
all wrap their rangeCheckI() calls in if(RANGECHECK). dxfAt3D called
them unconditionally.
No effect while RANGECHECK is 1: cubic_spline.o is byte identical before
and after, md5 b0b37b59a7cd4270e7014183ed032699. The point is what
happens when it is turned off for speed, which is the only reason the
macro exists. Compiling both versions with RANGECHECK 0:
dxfAt3D instructions "dxfAt3D,xc" etc in .o size
unguarded 46 3 9608
guarded 17 0 9144
So the unguarded version keeps paying for all three checks, and keeps
their message strings, in exactly the build where the user asked not to.
Note that gcc inlines rangeCheckI() at -O3, so there is no call
instruction to look for, which is why this is measured by instruction
count and string table rather than by disassembly search.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two branches were byte identical:
if((roi_size%2)==0){
fit_data->xoff = 0.5*((double)roi_size) - 1.0;
fit_data->yoff = 0.5*((double)roi_size) - 1.0;
}
else{
fit_data->xoff = 0.5*((double)roi_size) - 1.0;
fit_data->yoff = 0.5*((double)roi_size) - 1.0;
}
The comment above them explains why, and it is worth keeping: the author
tested even and odd ROI sizes and found the same offset optimal for
both. But a branch that does not branch reads as an unfinished edit, and
a reader has to compare the two bodies character by character to find
out that it is deliberate. Say it in the comment instead, which is where
it belongs.
Behaviour preserving, and provably so: dao_fit.o is byte identical
before and after, md5 b2dc5b13076ae6d61574c86d6345c5ad. gcc was already
collapsing this.
This is also what makes 3acb33f harmless, since both branches assign
xoff and yoff the same expression.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
loadDaoFitC() set daoInitializeZ.argtypes twice, two lines apart:
daofit.daoInitializeZ.argtypes = [ctypes.c_void_p]
daofit.daoInitializeZ.argtypes = [ctypes.c_void_p,
ndpointer(dtype=numpy.float64),
ndpointer(dtype=numpy.float64),
ctypes.c_double,
ctypes.c_double]
The first is overwritten immediately and never has any effect. The
second is the correct one: it matches dao_fit.h:75,
void daoInitializeZ(fitData *, double *, double *, double, double);
and the call site in MultiFitterZ.initializeC(), which passes five
arguments. Had the one argument version been the one that survived,
ctypes would have rejected every call.
Checked that this was the only such case. A scan of every *_c.py wrapper
for a ctypes argtypes or restype assigned more than once in the same
module finds nothing else; run against this file before the fix it does
report daofit.daoInitializeZ.argtypes, so it is capable of finding them.
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.
Six items with no behavioural effect on any current code path, listed
because each one costs a reader time working out whether it matters. One
commit per issue.
Nothing here can carry a regression test, so where a change touches C the
evidence is a byte-identical object file rather than a passing suite.
413d2c12,e9ffba5bMake multi_fit.h an accurate indexThe header was wrong in both directions at once.
mFitRemoveRunningPeaks()is defined, exported and called from Pythonthrough ctypes, but was the one
mFit*function missing a declaration.mFitIterateOriginal()was declared but has had no definition sincea19f27d4removed it in 2018. With both fixed, every definition inmulti_fit.chas a declaration and every declaration has a definition,38 each way.
cb24ec83Give HYSTERESIS its real value and use itHYSTERESISwas defined as 0.6 and never referenced, whilemFitUpdate()recentred AOIs using two literals. Reading the constant as its comment
describes it,
|dx - 0.5| > HYSTERESIS, the literals correspond to a valueof 1.0, not 0.6 — so hysteresis is live and the constant simply carried the
wrong number. Sets it to 1.0 and writes the bounds in terms of it.
multi_fit.ois byte identical before and after.863f51bcGuard dxfAt3D's range checks with RANGECHECKIts six siblings wrap their
rangeCheckI()calls inif(RANGECHECK);dxfAt3Dcalled them unconditionally, so turning range checking off forspeed left this one function still paying. Compiled with
RANGECHECK 0,dxfAt3Dgoes from 46 instructions to 17 and its three message stringsdisappear from the object.
cubic_spline.ois byte identical withRANGECHECK 1.ff68de75Collapse the identical if/else in daoInitializeBoth branches of
if((roi_size%2)==0)were byte identical. The commentabove them explains why — the author tested even and odd ROI sizes and
found the same offset optimal — so this keeps the explanation and drops the
branch that does not branch.
dao_fit.ois byte identical before and after; gcc was already collapsingit.
48fcac6cRemove the dead daoInitializeZ argtypes assignmentargtypeswas set twice two lines apart, the first immediately overwritten.The survivor is the correct five-argument version matching
dao_fit.h:75and the call site. A scan of every
*_c.pywrapper confirms this was theonly such case.
Full test suite passes, 277 tests.