Fix circular-coordinate handling and histogram support in NMI - #113
Merged
Conversation
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.
Summary
Two independent defects in the mutual-information pipeline, and they amplify each other.
Both affect NMI and therefore everything downstream: graph weights, path weights,
per-replica confidence, the block reference added in #112, and bootstrap path-confidence
intervals.
Defect A — the periodic coordinate was differenced without wrapping
MDAnalysis reports phi on
[-180, 180). A residue oscillating near the branch cut flipsbetween +179 and -179, so
np.diffturned a real 2 degree step into -358.Defect B — the histogram support was derived from each input's own extrema
Within a single analysis that is defensible adaptive binning. It becomes a defect the moment
NMI values from different inputs are compared — which is exactly what the multi-replica
confidence path, the block reference and the bootstrap all do. Two replicas of identical
dynamics, one containing a single larger excursion, get different bin widths, and that
difference flows straight into
MI_std->MI_cv->confidence.Changes
[-180, 180)instructure.py— the onlynp.diffin the package. Unconditional: this corrects a defect, not a configurable policy.
hist_rangeonNMICalculator, default(-180.0, 180.0), threaded throughEdgeConfidenceCalculator,BlockSplitNullandBootstrapAnalysisso a whole run sharesone binning. Added as the last parameter in each signature, so no positional call site
breaks.
-hist-range LOW HIGH(or-hist-range none) to set the support per run.-diagnose-binsandmdpath.src.mutual_information.binning_diagnostic(): per-residuehistogram occupancy under the chosen support, plus branch-cut exposure (frames with
|dphi| > 180before wrapping), written tobinning_diagnostic.csv.DataFrame.attrs["branch_cut_crossings"]and preservedacross the multi-trajectory column alignment (a bare
df[common_cols]dropped it).confidence.pyandnull_model.pythat per-replica and per-block valuesare only comparable under a fixed support.
Backwards compatibility
hist_range=Nonerestores the previous behaviour and is an exact no-op — it reproducespre-fix NMI values bit-for-bit, asserted in
test_hist_range_none_is_exact_no_op. Use it toreproduce published numbers.
Note the asymmetry:
Nonerestores only the binning. The dihedral wrap is unconditional,because an unwrapped periodic difference is simply wrong.
No CLI, output-file or column-name changes beyond the two new optional flags.
Two exact-value assertions in
test_mutual_information.pywere recomputed under the newdefault (largest change -3.3 % in the plain case; the
invertcase moves more becauserebinning changed which pair is the maximum). No assertion was weakened, and the pre-fix
values are retained in the backwards-compatibility test.
Tests
New regression tests cover:
|dphi| <= 180);hist_range=None;