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.

Sign up for GitHub

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

Jump to bottom

WIP/RFH: Make dims a keyword argument to reducing functions #25831

Closed
wants to merge 1 commit into from
Closed

WIP/RFH: Make dims a keyword argument to reducing functions #25831

wants to merge 1 commit into from

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 31, 2018 •
edited
Loading

Heavily WIP. If you're reading this, you can pretty much ignore this for now.

Still to do:

  • Fix the code
  • Fix the docs
  • Fix the tests
  • Fix inference in global scope
  • Get the other reducers, e.g. cum(sum|prod), var, std, etc.

Fixes #25501.

@ararslan ararslan added maths Mathematical functions collections Data structures holding multiple items, e.g. sets keyword arguments f(x; keyword=arguments) labels Jan 31, 2018
@ararslan ararslan requested a review from mbauman January 31, 2018 20:12
mbauman added a commit that referenced this pull request Jan 31, 2018
@mbauman
Test: compute at-inferred within a non-top-level context
5636f31 
The at-inferred macro, when presented with a kwargs call, previously would ask return types about the function directly. This is an accurate representation of how the call behaves at top/global scope, but typically we are much more interested in the behavior of the function when called from another function with well-typed arguments. Within this context, the compiler can use inlining and other optimizations that allow some keyword functions to recover type-stability. This is becoming a common idiom within base.

This patch changes the behavior of at-inferred to test the behavior of the `f(args..., kwargs...)` expression as though it were placed directly as written within an anonymous function.  Required for #25831.
@mbauman mbauman mentioned this pull request Jan 31, 2018
Closed
mbauman added a commit that referenced this pull request Jan 31, 2018
@mbauman
Test: compute at-inferred within a non-top-level context
7cf3586 
The at-inferred macro, when presented with a kwargs call, previously would ask return types about the function directly. This is an accurate representation of how the call behaves at top/global scope, but typically we are much more interested in the behavior of the function when called from another function with well-typed arguments. Within this context, the compiler can use inlining and other optimizations that allow some keyword functions to recover type-stability. This is becoming a common idiom within base.

This patch changes the behavior of at-inferred to test the behavior of the `f(args..., kwargs...)` expression as though it were placed directly as written within an anonymous function.  Required for #25831.
ararslan pushed a commit that referenced this pull request Feb 2, 2018
@mbauman @ararslan
Test: compute at-inferred within a non-top-level context
578b868 
The at-inferred macro, when presented with a kwargs call, previously would ask return types about the function directly. This is an accurate representation of how the call behaves at top/global scope, but typically we are much more interested in the behavior of the function when called from another function with well-typed arguments. Within this context, the compiler can use inlining and other optimizations that allow some keyword functions to recover type-stability. This is becoming a common idiom within base.

This patch changes the behavior of at-inferred to test the behavior of the `f(args..., kwargs...)` expression as though it were placed directly as written within an anonymous function.  Required for #25831.

(cherry picked from commit 7cf3586)
@ararslan
Copy link
Member Author

ararslan commented Feb 8, 2018

Inference issue, as requested by @JeffBezanson

Error During Test at /home/alex/Projects/julia/test/statistics.jl:198
  Got an exception of type ErrorException outside of a @test
  return type Float64 does not match inferred return type Any
  Stacktrace:
   [1] error(::String) at ./error.jl:33
   [2] macro expansion at /home/alex/Projects/julia/test/statistics.jl:227 [inlined]
   [3] macro expansion at /home/alex/Projects/julia/usr/share/julia/site/v0.7/Test/src/Test.jl:1008 [inlined]
   [4] top-level scope at /home/alex/Projects/julia/test/statistics.jl:199

@JeffBezanson JeffBezanson added this to the 1.0 milestone Feb 9, 2018
@JeffBezanson JeffBezanson self-assigned this Feb 9, 2018
@JeffBezanson
Copy link
Member

cumsum and cumprod only accept a single dimension --- should the argument be called dim in that case?

@JeffBezanson
Copy link
Member

Or I guess it could still be called dims since we might extend it to multiple dimensions at some point?

@ararslan
Copy link
Member Author

ararslan commented Feb 9, 2018

The inconsistency with sum and prod would be kind of weird, in my opinion, especially since you can also pass single dimensions to sum and prod. I'd call it dims across the board, even if it's technically improper English for single dimensions.

@mbauman
Copy link
Member

mbauman commented Feb 9, 2018

Consistency is required for an implementation of squeeze(f, A, dims=…) — #23500.

@JeffBezanson JeffBezanson mentioned this pull request Feb 10, 2018
Merged
@ararslan ararslan deleted the aa/dims-kwarg branch February 10, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman Awaiting requested review from mbauman

Assignees

@JeffBezanson JeffBezanson

Labels
collections Data structures holding multiple items, e.g. sets keyword arguments f(x; keyword=arguments) maths Mathematical functions
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.
3 participants
@ararslan @JeffBezanson @mbauman