Conversation
dance858
changed the title
Transurgeon
left a comment
Transurgeon
left a comment
There was a problem hiding this comment.
LGTM!
had a few questions, but feel free to merge after. Looks great!
| if (!node->value) | ||
| { | ||
| free(node); | ||
| return NULL; | ||
| } | ||
|
|
||
| // node->left = NULL; | ||
| // node->right = NULL; | ||
| // node->forward = NULL; | ||
| // node->jacobian = NULL; | ||
| // node->jacobian_init = NULL; | ||
| // node->eval_jacobian = NULL; | ||
| // node->is_affine = NULL; | ||
| // node->dwork = NULL; | ||
| // node->iwork = NULL; | ||
| // node->CSR_work = NULL; | ||
| node->var_id = -1; |
There was a problem hiding this comment.
I am wondering how this code was working previously? Maybe not important, but it would be nice if you could clarify.
My understanding is that these fields were just being initialized then populated later.
Now instead, they must be passed as arguments to the init_expr constructor.
| 1. power should be double | ||
| 2. can we reuse calculations, like in hessian of logistic | ||
| 3. more tests for chain rule elementwise univariate hessian | ||
| 4. in the refactor, add consts |
There was a problem hiding this comment.
is this done? Did you mean to add the type hints (I am not sure if that's what we call them) for the arguments to functions?
There was a problem hiding this comment.
I changed from "int p" to "double p" in power_expr, so now it's the correct data type
| // free_csr_matrix(lin_node->A_csr); | ||
| free_csc_matrix(lin_node->A_csc); |
There was a problem hiding this comment.
why do we have both csc and csr matrices? Could you explain.. so that I understand the comment above?
There was a problem hiding this comment.
The chain rule implementation becomes much more efficient if we can both traverse columns and rows of A fast. So this is just for efficiency (and also for simplifying the code).
| init_expr(node, A->m, 1, u->n_vars, forward, NULL, NULL, is_affine, | ||
| free_type_data); |
There was a problem hiding this comment.
I see where init_expr is being used now, please ignore my comments above!
| /* Initialize type-specific fields */ | ||
| lin_node->A_csr = new_csr_matrix(A->m, A->n, A->nnz); | ||
| copy_csr_matrix(A, lin_node->A_csr); | ||
| lin_node->A_csc = csr_to_csc(A); |
There was a problem hiding this comment.
I don't understand the changes here. Why are we converting csr to csc?
There was a problem hiding this comment.
We store A both in CSC and CSR for efficient chain rule implementation
2 participants
Cleaned up elementwise univariate and affine atoms, and implemented better polymorphism. Also refactored hstack to precompute sparsity pattern, which is pretty nice.