Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Aug 26, 2025

Summary

This PR addresses inconsistencies in PyTorch tensor operations and fixes a critical bug in the n-gram evaluation functionality that was causing incorrect predictions.

Issues Fixed

1. torch.nonzero() Inconsistency

The ngram() and update_ngram_scores() functions used different approaches to extract non-zero indices:

# ngram() function (inefficient)
ordering = torch.nonzero(activity[t].view(-1))
if ordering.numel() > 0:
    fire_order += ordering[:, 0].tolist()

# update_ngram_scores() function (efficient)
ordering = torch.nonzero(activity[t]).view(-1)
if ordering.numel() > 0:
    fire_order += ordering.tolist()

Both approaches produce identical results, but the second is more direct and efficient. This PR standardizes on the cleaner approach.

2. Critical N-gram Range Bug

The ngram() function had an off-by-one error in the n-gram sequence processing loop:

# Before (incorrect - misses last n-gram)
for j in range(len(fire_order) - n):

# After (correct - includes all possible n-grams)  
for j in range(len(fire_order) - n + 1):

This bug caused the function to skip the last possible n-gram sequence, potentially leading to incorrect classification predictions.

3. Modernized Tensor Operations

Updated deprecated tensor size access pattern:

# Before
for t in range(activity.size()[0]):

# After  
for t in range(activity.size(0)):

Testing

  • All existing tests continue to pass
  • Added comprehensive test coverage for n-gram functionality
  • Verified edge cases (empty spikes, single spikes, multiple spikes per timestep)
  • Confirmed backward compatibility

Impact

  • Correctness: Fixes incorrect predictions in n-gram classification
  • Performance: Slightly more efficient tensor operations
  • Maintainability: Consistent code patterns across functions
  • Compatibility: Ensures compatibility with PyTorch 2.7.1

The changes are minimal and surgical, maintaining full backward compatibility while improving correctness and efficiency.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] what updates are needed? Fix torch.nonzero inconsistency and n-gram range bug in evaluation.py Aug 26, 2025
@Copilot Copilot AI requested a review from Hananel-Hazan August 26, 2025 15:19
Copilot finished work on behalf of Hananel-Hazan August 26, 2025 15:19
Copy link
Collaborator

@Hananel-Hazan Hananel-Hazan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to maintine backwaord compatibility to PyTorch 2.7.1.
The code should work with Python >=3.9

@Hananel-Hazan Hananel-Hazan marked this pull request as ready for review August 26, 2025 16:47
@Hananel-Hazan Hananel-Hazan marked this pull request as draft August 26, 2025 16:48
@Hananel-Hazan Hananel-Hazan self-requested a review August 26, 2025 22:35
Copy link
Collaborator

@Hananel-Hazan Hananel-Hazan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to maintine backwaord compatibility to PyTorch 2.7.1.
The code should work with Python >=3.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants