Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow mixing offset and non-offset axes in conv input #586

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

martinholters
Copy link
Member

In reference to #583, but doesn't fix it. Rather, it explicitly throws an error for any input where we're not quite sure what it should do. That would leave us the opportunity to make a more informed choice later when (and if) concrete use cases for mixed offset/non-offset inputs are reported.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.84%. Comparing base (a98e64f) to head (4c19923).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
src/dspbase.jl 92.85% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #586   +/-   ##
=======================================
  Coverage   97.84%   97.84%           
=======================================
  Files          19       19           
  Lines        3249     3254    +5     
=======================================
+ Hits         3179     3184    +5     
  Misses         70       70           

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


🚨 Try these New Features:

src/dspbase.jl Outdated Show resolved Hide resolved
@martinholters martinholters force-pushed the mh/disallow-mixed-axes-conv branch from 97e6c31 to 4c19923 Compare November 15, 2024 07:18
@test_throws ArgumentError conv!(zeros(6), offset_arr, 1:3) # output needs to be OA, too
@test_throws ArgumentError conv!(OffsetVector{Int}(undef, 1:6), 1:4, 1:3) # output mustn't be OA

@test conv(fa, fill(true)) == conv(fill(true), fa) == fa
@test_broken conv(offset_arr_f, fill(true)) == conv(fill(true), offset_arr_f) == offset_arr_f
Copy link
Member Author

Choose a reason for hiding this comment

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

As fill(true) doesn't have any axes, this should arguably work. However, conv currently first brings all arguments to the same dimensionality, turning fill(true) into [true] here, and the axis compatibility check then says no. Fixing that is probably not important and certainly beyond the scope of this PR, but I thought leaving a @test_broken as a reminder might be in order.

Copy link
Member

@wheeheee wheeheee left a comment

Choose a reason for hiding this comment

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

LGTM, could also @test_throws for the ArgumentError

@martinholters
Copy link
Member Author

LGTM, could also @test_throws for the ArgumentError

For what currently is the @test_broken, you mean? Yeah, I'm not sure whether we'd like that to work or not. This way it's maybe clearer that we're still undecided.

@wheeheee
Copy link
Member

I was referring to conv_axis_with_offset(a::Any)

@martinholters
Copy link
Member Author

Ah, ok. Well, could of course directly test conv_axis_with_offset, like

@test_throws ArgumentError conv_axis_with_offset("foo")

But I don't think that adds much value. And for any axis that might actually appear in the wild, we might conceivably support it in the future, so not much point in explicitly testing that it errors now.

@wheeheee
Copy link
Member

Yeah that makes sense too. Merge?

@martinholters martinholters merged commit 3d87980 into master Nov 20, 2024
10 of 11 checks passed
@martinholters martinholters deleted the mh/disallow-mixed-axes-conv branch November 20, 2024 09:01
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