Skip to content

Fix axis ordering in read_region/write_region#433

Open
simonrw wants to merge 1 commit intomainfrom
fix/axis-ordering-region
Open

Fix axis ordering in read_region/write_region#433
simonrw wants to merge 1 commit intomainfrom
fix/axis-ordering-region

Conversation

@simonrw
Copy link
Owner

@simonrw simonrw commented Feb 10, 2026

Summary

  • create_image and fetch_hdu_info reverse dimensions to present them in C row-major order, but read_region and write_region were passing ranges directly to CFITSIO without reversing
  • This caused the ranges to be interpreted with swapped axes, making region operations inconsistent with the rest of the API
  • Updated test assertions to match the corrected axis ordering

Test plan

  • All 150 existing tests pass with corrected assertions
  • Verify read_region/write_region now use consistent row-major axis ordering with create_image/fetch_hdu_info

🤖 Generated with Claude Code

…nvention

create_image and fetch_hdu_info reverse dimensions to present them in C
row-major order, but read_region and write_region were passing ranges
directly to CFITSIO without reversing. This caused the ranges to be
interpreted with swapped axes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2026

📝 Walkthrough

Walkthrough

This pull request adjusts how image region reading and writing operations interpret user-provided ranges to align with CFITSIO's Fortran column-major convention. User-provided ranges are specified in C row-major order [rows, columns], but the implementation now reverses the iteration order to correctly handle the underlying column-major data layout. Test expectations and comments are updated accordingly to reflect this coordinate system interpretation change.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing axis ordering in read_region/write_region functions to align with the rest of the API.
Description check ✅ Passed The description comprehensively explains the issue, its impact on API consistency, and the solution implemented, and is directly related to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/axis-ordering-region

No actionable comments were generated in the recent review. 🎉


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.5%. Comparing base (83b76a5) to head (4ba6af5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
fitsio/src/fitsfile.rs 94.8% <100.0%> (ø)
fitsio/src/images.rs 96.7% <100.0%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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