Skip to content

Replace padded_similar by similar#634

Merged
mkitti merged 1 commit into
masterfrom
jishnub/padded_similar
Jun 5, 2026
Merged

Replace padded_similar by similar#634
mkitti merged 1 commit into
masterfrom
jishnub/padded_similar

Conversation

@jishnub

@jishnub jishnub commented Sep 30, 2025

Copy link
Copy Markdown
Member

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.

@codecov

codecov Bot commented Sep 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.09%. Comparing base (7a2d581) to head (35865d5).
⚠️ Report is 8 commits behind head on master.

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

mkitti commented Sep 30, 2025

Copy link
Copy Markdown
Collaborator

Why is this not breaking currently?

@jishnub

jishnub commented Sep 30, 2025

Copy link
Copy Markdown
Member Author

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

mkitti commented Sep 30, 2025

Copy link
Copy Markdown
Collaborator

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

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

Copilot AI left a comment

Copy link
Copy Markdown

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)

Copilot AI Oct 1, 2025

Copy link

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
Copy Markdown
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
Copy Markdown
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

jishnub commented Oct 1, 2025

Copy link
Copy Markdown
Member Author

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.

@jishnub

jishnub commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Gentle bump

@mkitti mkitti merged commit da82006 into master Jun 5, 2026
17 checks passed
@mkitti mkitti deleted the jishnub/padded_similar branch June 5, 2026 16:43
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.

3 participants