Skip to content

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Sep 30, 2025

These should be functionally equivalent, but this removes an explicit dependency on OffsetArrays. This change makes it easier to move OffsetArrays support to a package extension in a future breaking release.

Copy link

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.09%. Comparing base (7a2d581) to head (35865d5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
- Coverage   88.10%   88.09%   -0.02%     
==========================================
  Files          29       29              
  Lines        1908     1906       -2     
==========================================
- Hits         1681     1679       -2     
  Misses        227      227              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@mkitti
Copy link
Collaborator

mkitti commented Sep 30, 2025

Why is this not breaking currently?

@jishnub
Copy link
Member Author

jishnub commented Sep 30, 2025

This package is loading OffsetArrays, which, through type piracy, adds the extra method to similar that is equivalent to padded_similar. This is why this change doesn't impact anything, since the equivalent similar method already exists.

It is because of this type-piracy that it might be better to not depend explicitly on OffsetArrays in the future and use a package extension instead.

@mkitti
Copy link
Collaborator

mkitti commented Sep 30, 2025

Is it breaking to drop Offset Arrays as a dependency now?

@mkitti mkitti requested a review from Copilot October 1, 2025 03:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes a custom padded_similar function and replaces its usage with Julia's built-in similar function. The change aims to eliminate an explicit dependency on OffsetArrays to facilitate moving OffsetArrays support to a package extension in future releases.

  • Removes the padded_similar function definition that handled both regular arrays and offset arrays
  • Replaces the call to padded_similar(TC, indspad) with similar(Array{TC}, indspad)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

indsA = axes(A)
indspad = padded_axes(indsA, it)
coefs = padded_similar(TC, indspad)
coefs = similar(Array{TC}, indspad)
Copy link

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

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

The replacement similar(Array{TC}, indspad) may not be functionally equivalent to the original padded_similar(TC, indspad). The original function returned OffsetArray{TC}(undef, inds) for non-OneTo indices, but similar(Array{TC}, indspad) will always return a regular Array type regardless of the index type. This could break functionality when indspad contains offset indices.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that actually a problem based on how coefs is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well Copilot isn't correct here. similar will return an OffsetArray if the package is loaded, so there's no difference here.

@jishnub
Copy link
Member Author

jishnub commented Oct 1, 2025

I'm not familiar with this package to say whether it's breaking. Presumably it will be, as offset axes won't be supported by default if OffsetArrays isn't a dependency. A bigger question is whether this needs to be supported at all.

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