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

Use error-hint for fft-related errors #190

Merged
merged 1 commit into from
Jan 9, 2023
Merged

Use error-hint for fft-related errors #190

merged 1 commit into from
Jan 9, 2023

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 5, 2023

Aqua was complaining about piracy. This eliminates some of the
piracy it was worried about, and the rest we temporarily (?)
circumvent by disabling Aqua's piracy test.

This package now only loads AbstractFFTs on Julia versions
that lack Base.Experimental.register_error_hint (Julia 1.4
and earlier).

Aqua was complaining about piracy. This eliminates some of the
piracy it was worried about, and the rest we temporarily (?)
circumvent by disabling Aqua's piracy test.

This package now only loads AbstractFFTs on Julia versions
that lack `Base.Experimental.register_error_hint` (Julia 1.4
and earlier).
@timholy timholy mentioned this pull request Jan 5, 2023
@timholy
Copy link
Member Author

timholy commented Jan 5, 2023

I'm not wedded to this approach, so don't hold back with any concerns. We could also just disable the piracy test permanently.

If on the contrary you think this is an improvement, I can look into the other sources of piracy.

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Base: 62.67% // Head: 75.12% // Increases project coverage by +12.45% 🎉

Coverage data is based on head (9623cc1) compared to base (6fc57c1).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #190       +/-   ##
===========================================
+ Coverage   62.67%   75.12%   +12.45%     
===========================================
  Files          10       10               
  Lines         584      599       +15     
===========================================
+ Hits          366      450       +84     
+ Misses        218      149       -69     
Impacted Files Coverage Δ
src/ImageCore.jl 78.37% <100.00%> (+5.96%) ⬆️
src/functions.jl 0.00% <0.00%> (-100.00%) ⬇️
src/stackedviews.jl 79.74% <0.00%> (+0.25%) ⬆️
src/convert_reinterpret.jl 64.44% <0.00%> (+4.44%) ⬆️
src/precompile.jl 87.77% <0.00%> (+87.77%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

My understanding is that type piracy will affect invalidation and thus hurt future using XXX time. Will this still be true for Julia 1.9?

I like this change -- registering error hints by type piracy works but isn't a good choice.
If anything, I think it's a better idea to improve the error hint mechanism -- make it easier to add hooked information for developers. Although I have no idea what could it be.

@timholy
Copy link
Member Author

timholy commented Jan 9, 2023

My understanding is that type piracy will affect invalidation and thus hurt future using XXX time. Will this still be true for Julia 1.9?

Yes and no. For MethodInstances that are well-inferred, the only way piracy invalidates is if you overwrite a method with the same type-signature (you're directly replacing a call in the dispatch chain). For MethodInstances that have poorly-inferred calls (e.g., fft(::AbstractMatrix), then any new overlapping definition is at risk for invalidating whether it's piracy or not. So yes, piracy is more of an invalidation threat than non-piracy, but the way in which it's more of a threat is fairly specific.

I don't know of any packages that define fft(::AbstractArray{<:Colorant}), so in this case invalidation of well-inferred code is probably not very likely.

@timholy timholy merged commit 6d4b140 into master Jan 9, 2023
@timholy timholy deleted the teh/fft branch January 9, 2023 11:34
@timholy
Copy link
Member Author

timholy commented Jan 9, 2023

In my view this is not a breaking change (it only changes the kind of error thrown), so does not require a breaking release.

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