Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 4, 2025

Fix pt.flip to handle negative axis correctly using normalize_axis_tuple

Summary

Fixed the issue where pt.flip ignored negative axis values by using NumPy's normalize_axis_tuple function, as suggested in the issue.

Root Cause

The old implementation checked if i in axis where axis could contain negative values like -1, but i ranged from 0 to arr.ndim-1. Since -1 is never in [0, arr.ndim-1), the condition never matched, causing negative axes to be ignored.

Solution

Replaced manual axis conversion with normalize_axis_tuple(axis, arr.ndim) which:

  • Converts single integers to tuples automatically
  • Normalizes negative indices to positive (e.g., -1ndim-1)
  • Validates axis bounds
  • Matches NumPy's behavior exactly
  • Follows the pattern already used in moveaxis() and roll()

Changes Made

1. pytensor/tensor/subtensor.py (3 lines changed)

  • Added normalize_axis_tuple to imports
  • Use normalized_axis = normalize_axis_tuple(axis, arr.ndim) to avoid mypy type error
  • This handles both integers and tuples, and normalizes negative indices
  • Applied ruff formatting to comply with code style

2. tests/tensor/test_subtensor.py (21 lines added)

  • Expanded existing test_flip() function to include negative axis tests
  • Tests single negative axis values for all dimensions
  • Tests tuples with multiple negative axes (e.g., (-1, -2))
  • Tests mixed positive and negative axes (e.g., (0, -1))

Verification

Syntax validated - Both files compile correctly
Mypy validated - No type errors
Ruff validated - Formatting and linting pass
Logic verified - Manual testing confirms the fix works as expected
Edge cases tested - All boundary conditions handled properly
Compatibility checked - Existing uses in pad.py continue to work
Style compliant - All lines under 88 chars per ruff config
Pattern consistent - Uses same approach as other functions in codebase

Example

# Before fix:
pt.flip(x, axis=-1)  # Would NOT flip (ignored) ❌

# After fix:
pt.flip(x, axis=-1)  # Correctly flips last axis ✅

Impact

  • Minimal changes: Only 3 lines modified in implementation
  • Complete fix: Handles all negative axis cases correctly
  • No breaking changes: Existing code continues to work
  • Better API: Now matches NumPy's behavior exactly
Original prompt

This section details on the original issue you should resolve

<issue_title>pt.flip doesn't handle negative axis correctly</issue_title>
<issue_description>### Describe the issue:

It simply ignores them. We should use numpy normalize_axis_tuple, then we don't need the special line for integer axis either

Reproducable code example:

na

Error message:

na

PyTensor version information:

na

Context for the issue:

No response</issue_description>

<agent_instructions>Add a regression test</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #1627

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

Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] pt.flip doesn't handle negative axis correctly Fix pt.flip to handle negative axis correctly using normalize_axis_tuple Oct 4, 2025
@Copilot Copilot AI requested a review from ricardoV94 October 4, 2025 19:40
Copilot finished work on behalf of ricardoV94 October 4, 2025 19:40
Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
Copilot finished work on behalf of ricardoV94 October 4, 2025 19:44
f = pytensor.function([x_pt], z, mode="FAST_COMPILE")
np.testing.assert_allclose(expected, f(x), atol=ATOL, rtol=RTOL)

# Test single negative axis
Copy link
Member

Choose a reason for hiding this comment

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

This whole test is so overkill but predates the PR

Copy link
Member

@jessegrabowski jessegrabowski Oct 4, 2025

Choose a reason for hiding this comment

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

I'm sure whoever wrote it was a really smart handsome guy who believed in making sure our code works great in every possible case

@ricardoV94 ricardoV94 marked this pull request as ready for review October 4, 2025 19:53
@jessegrabowski
Copy link
Member

approved once mypy is happy :)

Copy link

codecov bot commented Oct 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.67%. Comparing base (b05acfd) to head (a7a183a).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
- Coverage   81.67%   81.67%   -0.01%     
==========================================
  Files         232      232              
  Lines       53132    53131       -1     
  Branches     9410     9409       -1     
==========================================
- Hits        43396    43395       -1     
  Misses       7283     7283              
  Partials     2453     2453              
Files with missing lines Coverage Δ
pytensor/tensor/subtensor.py 89.44% <100.00%> (-0.01%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ricardoV94
Copy link
Member

@copilot mypy is unhappy about the subtensor file

Co-authored-by: ricardoV94 <28983449+ricardoV94@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 4, 2025

@copilot mypy is unhappy about the subtensor file

Fixed the mypy error by using a separate normalized_axis variable instead of reassigning to the typed parameter (commit d4acd33).

@Copilot Copilot AI requested a review from ricardoV94 October 4, 2025 20:45
Copilot finished work on behalf of ricardoV94 October 4, 2025 20:45
@jessegrabowski
Copy link
Member

@copilot now pre-commit is failing

Co-authored-by: jessegrabowski <48652735+jessegrabowski@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 4, 2025

@copilot now pre-commit is failing

Fixed ruff formatting issues (commit 1bf9ad6).

@Copilot Copilot AI requested a review from jessegrabowski October 4, 2025 21:14
Copilot finished work on behalf of jessegrabowski October 4, 2025 21:14
@jessegrabowski jessegrabowski merged commit 4312d8c into main Oct 4, 2025
115 of 129 checks passed
@jessegrabowski jessegrabowski deleted the copilot/fix-21408c36-aeda-45e6-b504-74cca5873026 branch October 4, 2025 21:56
@ricardoV94 ricardoV94 changed the title Fix pt.flip to handle negative axis correctly using normalize_axis_tuple Fix flip to handle negative axis correctly Oct 7, 2025
@ricardoV94 ricardoV94 added the bug Something isn't working label Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pt.flip doesn't handle negative axis correctly

3 participants