Skip to content

Conversation

@twiecki
Copy link
Contributor

@twiecki twiecki commented Sep 27, 2020

Closes #49.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some tests.

@twiecki
Copy link
Contributor Author

twiecki commented Sep 28, 2020

Thanks, how do I handle kwargs?

@brandonwillard
Copy link
Member

Thanks, how do I handle kwargs?

Which kwargs? For the jax functions?

In general, the jax function arguments are going to come from either the arguments to the JAX closure/function constructed in the Op dispatch function (e.g. jax_funcify_ExtractDiag)—which themselves are determined by the arguments passed to the inputs argument of the Op's perform method—or from fields on the Op instance itself. In the latter case, those fields simply need to be added to the constructed closure/function via reference to the original Op object or—more preferably—by use of default keyword values in the closure/function (e.g. def jax_funcify_ExtractDiag(..., axis=op_axis_value)) where op_axis_value = op.axis preceding the function definition).

@brandonwillard brandonwillard changed the title Add new Ops, still experimental. Add ExtractDiag, Cholesky, and Solve Ops to jaxification Sep 28, 2020
@brandonwillard
Copy link
Member

Just pushed some changes. Needs a few more tests, though.

@twiecki
Copy link
Contributor Author

twiecki commented Sep 29, 2020

Added more tests, I think this covers everything. Not sure why the lower=False doesn't work.

@twiecki
Copy link
Contributor Author

twiecki commented Sep 29, 2020

OK, figured out lower.

@twiecki
Copy link
Contributor Author

twiecki commented Sep 29, 2020

Dang, messed up some commits when rebasing. Closing in favor of #59.

@brandonwillard
Copy link
Member

I think it would be easier if we just wait for the test on this one to finish, merge it, then rebase #59; especially since #59 is behind this branch.

@brandonwillard brandonwillard deleted the add_diag_chol_solve branch October 5, 2020 23:27
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.

Missing Op: ExtractDiag

3 participants