Normal cdf + copy args in smooth canonicalizer#178
Conversation
|
Parth, what do you think about the name "normal_cdf"? |
|
MATLAB uses normcdf and SciPy uses norm.cdf and torch uses normal.cdf and JAX uses norm.cdf Fortran used cdf_normal, R uses ??? pnorm ??? My soft preference is to call it normcdf to match log_normcdf, but as long as we don't use what R does I'm happy. https://www.mathworks.com/help/stats/normcdf.html |
Haha great! I'll call it normcdf then. |
|
Benchmarks that have stayed the same: |
| def smooth_full_domain_canon(expr, args): | ||
| if isinstance(args[0], Variable): | ||
| return expr, [] | ||
| return expr.copy([args[0]]), [] |
There was a problem hiding this comment.
can you add a comment about this in the PR title? After that I think its good to merge.
There was a problem hiding this comment.
also we should ensure that all full domain atoms have only one argument (since it indexes with 0), but this should be fine.
There was a problem hiding this comment.
I think they always have one argument, right?
There was a problem hiding this comment.
I thought that was part of the contract of being a full domain function and the multiargs had different requirements
There was a problem hiding this comment.
If we wanted to be safe we could just pass args directly tho!
This PR adds the normal cdf as an atom. It also fixes a subtle error that was raised when we called a smooth full domain atom on a division, like exp(1/x).