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

Unary Vector Functions - Vectorisation framework #12

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Unary Vector Functions - Vectorisation framework #12

merged 3 commits into from
Oct 9, 2023

Conversation

andrjohns
Copy link
Contributor

Summary

As first discussed in this math issue, this design document outlines my proposed approach to extended unary vector functions to work with row vectors, std::vectors, Eigen expression templates, and std::vectors of these.

A new vectorisation framework, apply_vector_unary, is proposed. The framework has three functions to address the different types of vector functions:

  • apply_vector_unary<T>::apply() for: f(vector) -> vector
  • apply_vector_unary<T>::reduce() for: f(vector) -> scalar
  • apply_vector_unary<T>::apply_scalar() for: f(vector, scalar) -> vector

Rendered version here

mitzimorris
mitzimorris previously approved these changes Dec 15, 2019
Copy link
Member

@mitzimorris mitzimorris left a comment

Choose a reason for hiding this comment

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

lgtm

@mitzimorris
Copy link
Member

hi Andrew, added you to this repo.

@SteveBronder
Copy link
Collaborator

Thanks Andrew! I think the design concepts make sense here. Would we want to include partial reductions, visitors, and broadcasts like eigen does here

I have some Qs around implementation, but I think those should wait until the actual PR (less you want them here)

@andrjohns
Copy link
Contributor Author

Would we want to include partial reductions, visitors, and broadcasts like eigen does here

That would be a good idea. I don't think the broadcasts are relevant for the unary functions, but they would be useful for binary functions (on the eventual to-do list).

For the partial reductions (i.e. colwise and rowwise) the real question/conundrum is going to be in how implement and expose the functionality. My first guess would be to have something like log_softmax(m.colwise()), but then that won't work for std::vector<MatrixXd>. The other approaches could be separate functions (e.g. log_softmax_colwise(m)), but then that just leads to all the extra definitions I was trying to avoid. Alternatively there could be flags in the call itself, like log_softmax(m, 1), where 0 = as-is, 1 = colwise, 2 = rowwise. But that isn't very intuitive to look at and would require changing the function wherever it's currently used.

Overall, I think it would be great to have in, but I'm not sure how to do it 'right'

@andrjohns
Copy link
Contributor Author

I have some Qs around implementation, but I think those should wait until the actual PR (less you want them here)

Implementation Qs would be very welcome! Good to get the design in an agreed-upon state before I start using up testing resources with a PR

@seantalts
Copy link
Member

hi Andrew, added you to this repo.

Hey all, the intention behind this repo is to have a separate set of people who can approve design docs; it shouldn't be the same list of people that can approve pull requests in a given domain. The reason for this is that design docs are used for major changes that often require coordination across an entire domain or multiple domains. I'm going to remove the folks who haven't been explicitly okayed by a tech lead for a specific domain. Sorry for the confusion!

@bob-carpenter
Copy link
Collaborator

Not sure where I'm supposed to interact with these cross-media discussions, so I commented on Discourse and am linking from here.

@andrjohns
Copy link
Contributor Author

@rok-cesnovar would you mind re-approving and merging this? It was approved way back but I never actually merged (the actual Math framework has already been implemented). I just had to rename the file since there was now a name clash with another 0004 design

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

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

Re-approving for procedural reasons.

@rok-cesnovar rok-cesnovar merged commit 8eede8b into stan-dev:master Oct 9, 2023
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.

6 participants