Some bug fixes and hardening in the Campbell diagrams - #23
Open
bjonkman wants to merge 11 commits into
Open
Conversation
The second term of the first denominator factor summed md2.EigenVector[i] * md2.EigenVector[i] instead of the md1 product, so the factor evaluated to (phi1^H phi1 + |phi2^T phi2|) rather than (phi1^H phi1 + |phi1^T phi1|). That made the criterion asymmetric in its arguments and allowed it to exceed 1, which matters because connectModesMAC and spectralClustering compare modes in whichever order they happen to iterate. Checked against the formula from the referenced ISMA 2010 paper over 5000 random complex mode shapes. The previous expression disagreed with itself under argument swap on every pair, with a largest discrepancy of 1.70, and fell outside [0, 1] on 217 of them. With the fix the criterion matches the reference, is symmetric, stays within [0, 1], and returns exactly 1 for a mode compared against itself. Assisted-by: Kiro:claude-opus-5
Three defects in the minimum cost assignment implementation. Step1 held a pointer into the row it was about to modify rather than a copy of the row minimum. Once the subtraction loop reached the position of the minimum, that element became 0, so every later element in the row had 0 subtracted instead of the row minimum. That is not a uniform row offset, so it changes which assignment is optimal rather than only shifting the dual variables. Against an exhaustive search over 3000 random square matrices the old code returned a suboptimal assignment 422 times. Step5 wrote into a path buffer of N entries, but the alternating series of primed and starred zeros reaches 2N-1 entries (N primes and N-1 stars). This panicked with an index out of range on a 5x5 cost matrix reached through MinCostAssignment, and on 170 of 6000 random matrices. The reference implementation allocates 2N. MinCostAssignment indexed cost[0] without checking for an empty matrix, so a caller with nothing to assign panicked instead of getting an empty result. Also replaced ':=' with '=' on Step4's findZero call. The declaration shadowed the outer row and col, which made the later 'col = star_col' dead and restarted every search from (0, 0). This one is not a correctness fix: over 3000 random matrices the shadowed version still returned an optimal assignment every time, so it removes redundant rescanning and brings the step back in line with the reference. Verified by comparing against exhaustive search over 4000 random cost matrices up to 6x6, including rectangular shapes. Assisted-by: Kiro:claude-opus-5
…g points Unpaired modes were tracked in a map keyed by column index, and the loop that turned leftovers into new mode sets ranged over that map. Go randomizes map iteration order, so identical input produced different ID and Label assignments from run to run. Confirmed on a two operating point case with two unpaired modes, where labels 2 and 3 swapped between consecutive runs. The candidate modes are now held in a slice and walked in index order, with a parallel bool slice marking the ones that paired. An operating point whose modes were all filtered out left modeSets empty, and the following mat.NewDense call panicked with "zero length in matrix dimension". Candidate modes are now gathered up front, an operating point with no candidates is skipped, and if no mode sets exist yet each candidate seeds one. Sizing the weight matrix from len(op.Modes) rather than the number of candidates also left trailing all-zero columns that mat.Max could report as the maximum, and dividing by a zero maximum produced int(NaN) in the cost matrix, which the language leaves undefined. Both are now avoided. These two are hardening; I did not find an input where they change the resulting mode sets. Assisted-by: Kiro:claude-opus-5
A mode with no MAC correlation to any other mode in its group has a zero degree, so 1/sqrt(di) stored +Inf in D^-1/2 and NaN propagated through Lsym and the eigen solve. The damage was silent rather than a crash: on a group of two mode sets containing one such mode, both sets came back with zero modes instead of their assigned modes. The scaling is now left at zero for those rows. Rows of the feature matrix are only normalized when their norm is non-zero. Scaling by 1/0 fills the observation with NaN, and because every comparison against NaN is false, the point lands in whichever cluster is checked first rather than the nearest one. Also corrected the comment above the eigenvalue sort, which said largest to smallest while the comparator sorts smallest first. The smallest eigenvalues of the symmetric Laplacian carry the cluster structure, so those are the ones that belong in the feature matrix. No behavior change. Assisted-by: Kiro:claude-opus-5
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
2 participants
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.
As I was doing some work on Campbell diagrams and comparing my Matlab implementation with ACDC, AI identified and fixed a few bugs, which I am sharing here. I also merged the
mainbranch intodev.A few would result in NaN or Inf, and some could have led to some incorrect sorting of modes.
The comments in the code are a little verbose, so feel free to edit if you feel like it.
Assisted-by: Kiro:claude-opus-5