pyxu-org / pyxu

Modular and scalable computational imaging in Python with GPU/out-of-core computing.
https://pyxu-org.github.io/
MIT License
117 stars 17 forks source link

[v2_kaan_review] Update ufunc and test_ufunc and implement all activation functions #25

Closed okumuskaan closed 1 year ago

okumuskaan commented 2 years ago
matthieumeo commented 2 years ago

Hi @okumuskaan, thanks for this PR, lots of work covered there! There are a few things that will need to be fixed before I can approve the PR. Here is a list:

  1. The documentation should be improved. In particular, please document the helper functions (e.g. sin for Sin) as well as the non standard arguments specific to certain classes such as base for Exp/Log. Also give in a Notes section (Numpy docstring style) an expression of the derivative of the ufunc and the values of the lipschitz/diff_lipschitzconstants when known (so that the user knows if the latter are available). Make sure you use LaTeX from within the ..math :: of :math: balises from Sphinx. Finally, add a See Also (Numpy docstring style) section where you point to classes from within the same family (e.g. cos points to other trigonometric functions).
  2. At the top level of ufunc.py make a docstring and explain/provide examples on how to use the classes/functions of the module in practice (i.e. Sin(shape) * Op vssin(op) for example).
  3. Decorate all Jacobian methods with enforce_precision, otherwise the input arr used to create Jacobian matrices will not be coerced to the requested type. Similarly self._base should be coerced with pycsou.runtime.coercein Exp/Log.apply/adjoint otherwise it will recast the whole computation to the wrong precision.
  4. I think the computation of the Jacobian matrix of Prod can be simplified to:
    @pycrt.enforce_precision(i="arr")
    def grad(self, arr: pyct.NDArray) -> pyct.NDArray:
        xp = pycu.get_array_module(arr)
        g = xp.repeat(x[..., None, :], x.shape[-1], axis=0)
        e = xp.eye(*g.shape[-2:]).broadcast_to(g.shape).astype(bool)
       g[e] = pycrt.coerce(1.)
       return xp.prod(g, axis=-1)

    With this implementation you do not need explicit handling of zeros as you do in _grad_row.

  5. For Cumprod/sum the argument axis should come with an argument argshape: by convention, Pycsou always acts on the last axis of input arrays so there is theoretically no need to provide an axis keyword. The latter can however be useful if the stacks (...,N) of input arrays can be reinterpreted as stacks of flattened multidimensional inputs (..., *argshape). We should discuss that at the next dev meeting (@SepandKashani).
  6. Try to simplify Cumprod.jacobian using the simplifiedProd.jacobian implementation above.
  7. Clip should be a DiffMap (it is almost everywhere differentiable, so numerically we can consider it is differentiable just like Abs). Provide its derivative (1 over [a_min, a_max] and 0 elsewhere) and lipschitz/diff_lipschitz constants. Same for Sign.
  8. For activation functions, provide the mathematical definition of the function in the docstring (not everyone knows what a sigmoid or GELU are for example).
  9. Implement ReLU via Clip with a_min, a_max = 0, None.
  10. GELU.apply is not module agnostic: you use erf from Scipy which only works with Numpy. Make sure your implementation works for CuPy/Dask too.
  11. Remove StopCriterion_LSQR from this PR please .
  12. Adapt test_ufunc.py to reflect all the changes above.

That's it, let me know if anything is unclear, I'd be happy to help. And thanks again for the much appreciated work.

okumuskaan commented 2 years ago

Summary of Last Commits

okumuskaan commented 2 years ago
SepandKashani commented 2 years ago

Closed by mistake.