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

Ensure broadcasted functions retain their bounds checks #64

Merged
merged 2 commits into from
Aug 19, 2021

Conversation

c42f
Copy link
Collaborator

@c42f c42f commented Aug 19, 2021

The inbounds assertion within the map! implementation, combined with
propagate_inbounds on gettokenvalue(d::BroadcastedDictionary, t) leads
to removing the bound check on any user-defined function which is
broadcasted. This adds an extra layer of dispatch to protect the
user-defined function which is broadcasted.

Fixes #63

Copy link
Owner

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Haha awesome - thanks for finding this one.

(Note: the original intent for _f(d) was to be an alias for d.f in the case that I overload getproperty on AbstractDict (as a shortcut for dictionaries with Symbol keys) so perhaps it could be renamed?)

The inbounds assertion within the map! implementation, combined with
propagate_inbounds on gettokenvalue(d::BroadcastedDictionary, t) leads
to removing the bound check on any user-defined function which is
broadcasted. This adds an extra layer of dispatch to protect the
user-defined function which is broadcasted.
@c42f c42f force-pushed the cjf/fix-broadcasted-inbounds branch from 5dbd4ca to 477e6f9 Compare August 19, 2021 06:08
@c42f
Copy link
Collaborator Author

c42f commented Aug 19, 2021

Note: the original intent for _f(d) was to be an alias for d.f in the case that I overload getproperty on AbstractDict (as a shortcut for dictionaries with Symbol keys) so perhaps it could be renamed

Yeah I figured, but I was lazy 😆. I renamed it to _call_f and kept _f as it was.

Also fixed the tests by making sure only is defined even on older versions of Julia.

@andyferris
Copy link
Owner

Great! 👍

There seems to be an UndefVarError in the CI.

@c42f
Copy link
Collaborator Author

c42f commented Aug 19, 2021

UndefVarError in the CI

This was weird. I think it may have been something to do with only being defined in an inner scope in an if block.

Anyway, it turns out that those tests always passed regardless because CI runs with --bounds-check=yes. So I move that test into its own file which can be run without the --bounds-check argument.

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #64 (7e96aba) into master (6918361) will decrease coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   74.06%   74.02%   -0.05%     
==========================================
  Files          19       19              
  Lines        1720     1721       +1     
==========================================
  Hits         1274     1274              
- Misses        446      447       +1     
Impacted Files Coverage Δ
src/broadcast.jl 50.76% <50.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6918361...7e96aba. Read the comment docs.

@andyferris
Copy link
Owner

Anyway, it turns out that those tests always passed regardless because CI runs with --bounds-check=yes. So I move that test into its own file which can be run without the --bounds-check argument.

Duh! 🤦‍♂️ Nice solution.

@andyferris andyferris merged commit 03d79e4 into master Aug 19, 2021
@andyferris andyferris deleted the cjf/fix-broadcasted-inbounds branch August 19, 2021 22:45
@c42f
Copy link
Collaborator Author

c42f commented Aug 20, 2021

Nice solution.

I stole it from Base 😆

i think it might be nicer if the whole set of tests ran in CI both with and without --bounds-check=yes, but it turns out this isn't so easy to do without a certain amount of hacking. julia-actions/julia-runtest#46 should make it a lot easier.

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.

Broadcasting only omits checks due to inbounds propagation
2 participants