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

Fix undefined nested iterating variable when using Base.Generators and iterating over vector of functions #140

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

GianlucaFuwa
Copy link
Contributor

Fixes #108 and #108 (comment) by modifying extractargs! to recognize generators and handle them accordingly and checking if a function to be called is defined in the caller's module.

I am not 100% sure on this one, so possible improvements are greatly appreciated.

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.17%. Comparing base (1296bb3) to head (e870b29).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   93.29%   93.17%   -0.13%     
==========================================
  Files           3        3              
  Lines         582      586       +4     
==========================================
+ Hits          543      546       +3     
- Misses         39       40       +1     

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

@chriselrod chriselrod merged commit a096315 into JuliaSIMD:master Apr 1, 2024
28 of 31 checks passed
@chriselrod
Copy link
Member

Looks good to me!
But I'd really suggest a different code pattern for the functions in general.
In this particular example, the functions are actually all the same type, but more generally, I'd recommend making it a tuple, and then maping over it.

Something like:

_apply(f, args...) = f(args...)
fs = (exp, log, sqrt)
map(Base.Fix2(_apply, 2.3), fs)

Then you could sum the map or work with it in some other type stable way.

@GianlucaFuwa
Copy link
Contributor Author

_apply(f, args...) = f(args...)
fs = (exp, log, sqrt)
map(Base.Fix2(_apply, 2.3), fs)

Interesting, I didn't even know of Base.Fix's existence.

I myself haven't yet used generators within multithreading constructs but found that a fix to the issues would not take long, so I did it anyway.

@chriselrod
Copy link
Member

chriselrod commented Apr 1, 2024

Interesting, I didn't even know of Base.Fix's existence.

It isn't really needed here. What they do is roughly

Base.Fix1(f,x) = y -> f(x,y)
Base.Fix2(f,y) = x -> f(x,y)

except they return a struct, so typeof(Base.Fix2(f,y)) === Base.Fix2{typeof(f),typeof(y)}, instead of creating new types per anonymous function instance.

The important bit here is that map on tuples is generally type stable, even if each element of the tuple has a different type.
A loop won't be in the same circumstance.
Functions generally have different types, so if you have a tuple of functions, every element will likely have a different type.

That's not the case in @SouthEndMusic's example, but likely to be for whatever real thing that motivated it.

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.

Undefined nested iterating variable
2 participants