Fix subtle logic bugs and remove duplicate definitions#2
Open
chrishwiggins wants to merge 1 commit intoFrancoisSimon:mainfrom
Open
Fix subtle logic bugs and remove duplicate definitions#2chrishwiggins wants to merge 1 commit intoFrancoisSimon:mainfrom
chrishwiggins wants to merge 1 commit intoFrancoisSimon:mainfrom
Conversation
- Fix off-by-one in anomalous_diff_2D: angles[i-1] wraps to last element when i=0, should be angles[i] - Fix padding() referencing loop variable for dtype instead of track_list[0] - Change raise Warning(...) to warnings.warn(...) in padding() - Copy colnames list in read_table() to avoid mutating caller's list - Narrow bare except to except Exception and include error message - Remove duplicate transpose_layer class definition - Remove duplicate MLE_loss function definition Co-Authored-By: Claude Opus 4.6 <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.
Summary
Hi Francois - this is a companion to PR #1 (which covered the runtime crashes). This one addresses a few subtler issues that wouldn't necessarily crash but could produce unexpected behavior or make debugging harder down the road.
I've tried to keep each change as small and self-contained as possible. Happy to discuss any of these if you see them differently.
Changes
1. Off-by-one in
anomalous_diff_2Dangle indexing (line 211)The loop
for i in range(len(positions)-1)usedangle = angles[i-1]. Wheni=0, Python's negative indexing meansangles[-1]reads the last element of the array rather than the first. Sinceangles[0]is the randomly drawn initial heading and theanglesarray has exactlylen(positions)-1entries (one per displacement step), the intended indexing appears to beangles[i].This would affect the direction of the very first displacement step in simulated 2D tracks. For tracks with many sub-steps it might be hard to notice, but it could matter for short tracks or when the initial direction is important.
2.
padding()dtype from loop variable (line 336)mask = np.zeros(..., dtype = track.dtype)used the variabletrackwhich at that point refers to the last element iterated over in the precedingfor track in track_listloop. This works fine in practice (all tracks have the same dtype), but it's fragile - if track_list were empty, it would reference a variable from an outer scope or raise a NameError. Changed totrack_list[0].dtypeto make the intent clear.3.
raise Warning(...)changed towarnings.warn(...)(line 346)raise Warning(...)is syntactically valid Python, butWarningisn't typically used as an exception to raise. The intent here is clearly to warn the user about short tracks that get skipped, not to halt execution. Changed towarnings.warn(...)so that processing continues and users still see the message. Addedimport warningsat the top.4.
read_table()mutating the caller'scolnameslist (line 396)When tracks have composite IDs (the
colnames[3]is a list), the function doescolnames[3] = 'unique_ID'which modifies the list object that was passed in. If someone callsread_tablemultiple times with the samecolnameslist, the second call would see'unique_ID'instead of the original column names. Addedcolnames = list(colnames)at the top of the function to work on a copy.5. Bare
except:clause inread_table()(line 440)The bare
except:catches everything (includingKeyboardInterrupt,SystemExit, typos causingNameError, etc.) and only prints the filename with no indication of what went wrong. Narrowed it toexcept Exception as e:and included the error message in the print so that when files do cause problems, there's some context for debugging.6. Duplicate
transpose_layerclass definition removedThe class was defined identically at two locations in the file. The first definition (around line 1348) is the one used by
build_modelandbuild_abrupt_directed_motion_changes_model. Removed the second copy.7. Duplicate
MLE_lossfunction definition removedMLE_losswas defined twice in the module with identical implementations. The first definition (around line 2009) is the one that's exported and used. Removed the second copy so there's a single source of truth.Test plan
python -m py_compile exatrack.pypasses (confirmed locally)anomalous_diff_2Dand verify first displacement step direction is consistent with initial angleread_tabletwice with the samecolnameslist and verify it isn't modifiedpadding()with short tracks issues a warning instead of crashingGenerated with Claude Code