-
Notifications
You must be signed in to change notification settings - Fork 27
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
specialize tweight on FixedPoint #89
Conversation
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
==========================================
+ Coverage 93.15% 93.19% +0.04%
==========================================
Files 7 7
Lines 292 294 +2
==========================================
+ Hits 272 274 +2
Misses 20 20
Continue to review full report at Codecov.
|
I then realize that I don't have write permission to this repository when I'm about to merge this PR.. |
If upstream package (e.g., Interpolations, FixedPointNumbers) doesn't provide such specialization, then we add one here. This trick avoids incremental compilation warnings
end) | ||
# Reached when upstream package (e.g., FixedPoint) doesn't provide a specialization | ||
Interpolations.tweight(A::AbstractArray{T}) where T<:FixedPoint = T | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I can come up with so far to minimize the danger of type piracy (and incremental compilation). Do you have any better ideas? @timholy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is really an issue, I suspect we need to add a Requires
dependency to Interpolations and put this in a @require
block there. This solution seems likely to be fragile with respect to load order, and may not be fully correct wrt precompilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we need to add a Requires dependency to Interpolations and put this in a
@require
block there
This is more reasonable for me; I'll open a PR there.
may not be fully correct wrt precompilation.
Requires
will disable the precompilation for this line of code, and thus won't get any trouble, am I understanding it correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I'm also not completely clear on whether the precompiled module would retain this runtime check of the method tables; I think it wouldn't, it would just store the answer at the time when it happened to run. In contrast, by putting it into the __init__
method it's guaranteed to be correct.
This one is possibly no longer needed by Augmentor, so I'm closing it. |
Currently, The return eltype of
ImageTransformations.box_extrapolation(img, Flat())
isGray{N0f8}
forGray{N0f8}
input butFloat64
forN0f8
. To keep consistency, I add a specialization method here. (Technically it's a type piracy but I think it's safe to do so here)C.f. Evizero/Augmentor.jl#39
cc: @Evizero