Repository navigation
Check impose_resolution against the rules transcribed from the Fortran - #21
Merged
Merged
Conversation
scsim.impose_resolution had no test that owed nothing to it. Its behaviour was pinned against values it produced itself, which fixes whatever it does rather than whether what it does is right. RESHJC2.FOR in DCPROGS/DCFORTRAN imposed the dead time for every published HJCFIT result, Colquhoun, Hatton & Hawkes (2003) among them, and its rules are short enough to transcribe faithfully: RESOLV, lines 1045-1061: an interval is resolvable when its duration is at least treso if open or tresg if shut, and HJCFIT's simulate-and-fit path sets tresg = treso, so there is one dead time; the shut branch, line 419: a concatenated shut group ends only when the next interval is open AND resolvable; the open branch, line 448: an open group ends only when the next interval has a different amplitude, or is shut, AND is resolvable. Both readings are Colquhoun & Sigworth: an unresolvable event is absorbed into the interval in progress rather than split between neighbours. The two implementations agree interval for interval on five simulated records of 20000 intervals each and at five dead times from 1 to 500 microseconds. One difference is real, and is asserted rather than hidden. RESHJC2 lines 364-375 concatenate an unresolvable FIRST interval with the second and give the result the second's conductance class, where impose_resolution keeps it as its own interval. It can affect only the first interval of a record, and burst segmentation discards a leading shut interval anyway, so it has never mattered in practice -- but it is a difference, and a test now says so. If it ever starts failing, one of the two has changed its mind about the start of a record. Also pins the properties the rule exists to guarantee: every interval after the first is at least the dead time, the record still alternates, total time is conserved, and a dead time of zero changes nothing. 14 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
remislp
added a commit
that referenced
this pull request
Sep 11, 2026
Seven merged pull requests have been waiting since 1.1.0, and one of them is now blocking work elsewhere: HJCFIT's fitting layer is duck-typed on a mechanism and needs Rate.is_free, which exists here on master and not in the published 1.1.0. Its [fitting] extra pins scalcs>=1.1.0, which is too loose -- the integration tests fail on the released wheel with "'Rate' object has no attribute 'is_free'". A minor rather than a patch release, because it corrects computed values as well as adding API. #17 root bracketing for the asymptotic pdf. Counts were not recomputed when a search bound moved, widening happened once where it needed to repeat, and the upper bound was compared against the wrong state count. det[W(s)] is also unusable at large |s|, where it is of order 1e53 and changes sign hundreds of times in noise, so roots are now refined by bisecting the eigenvalue count where the determinant cannot be trusted. Roots are validated before being returned. #18 scsim re-exports burst segmentation from dcio; one implementation in the stack rather than two. #20 the dARSdS sign. exact_mean_open_shut_time was too large by exactly twice the dead time and the conditional means by once. This changes numbers a caller will have seen. #21 impose_resolution checked against RESHJC2.FOR interval for interval. #22 the simulator checked against Q-matrix theory for multi-state mechanisms, not only the two-state case where a reciprocal rate is the answer. #23 qmatlib.H inverted sI - QFF directly, which fails where the root search walks. The singularity is removable; only the factored form has a pole. #24 GPL-2.0-or-later rather than GPL-2.0-only, without which HJCFIT (GPL-3.0-or-later) could not depend on this at all. Both version files are bumped; the versions-agree gate added in #16 is what makes that hard to get wrong. Verified: the built wheel reports 1.2.0, carries its three data files, declares GPL-2.0-or-later, and Rate.is_free is present when installed from it into a clean environment. 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
1 participant
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.
scsim.impose_resolutionhad no test that owed nothing to it. Its behaviour was pinned against values it produced itself, which fixes whatever it does rather than whether what it does is right.RESHJC2.FORin DCPROGS/DCFORTRAN imposed the dead time for every published HJCFIT result, Colquhoun, Hatton & Hawkes (2003) among them. Its rules are short enough to transcribe faithfully, and this checksimpose_resolutionagainst that transcription.The rules
RESOLV, lines 1045–1061tresoif open ortresgif shut — and HJCFIT's simulate-and-fit path setstresg = treso(Hjcfit1-09122003.forline 2670), so there is one dead timeBoth are the Colquhoun & Sigworth definition: an unresolvable event is absorbed into the interval in progress rather than split between neighbours.
Result
The two implementations agree interval for interval on five simulated records of 20 000 intervals each, and at five dead times from 1 to 500 µs.
One real difference, asserted rather than hidden
RESHJC2lines 364–375 concatenate an unresolvable first interval with the second and give the result the second's conductance class, whereimpose_resolutionkeeps it as its own interval.It can affect only the first interval of a record, and burst segmentation discards a leading shut interval anyway, so it has never mattered in practice — but it is a difference, and
test_first_interval_is_the_one_known_differencesays so. If that ever starts failing, one of the two has changed its mind about the start of a record.Also pinned
The properties the rule exists to guarantee: every interval after the first is at least the dead time, the record still alternates open and shut, total time is conserved, and a dead time of zero changes nothing.
14 tests. Full suite: 604 passed, 1 skipped.
Written while reproducing Colquhoun, Hatton & Hawkes (2003), where the same transcription showed 0 differences over 196 988 intervals — this brings that check into the repository so it keeps being run.
🤖 Generated with Claude Code