Improve creating an Op documentation page #1086
Conversation
ricardoV94
changed the title
| @@ -165,35 +151,6 @@ or :meth:`Op.make_thunk`. | |||
| :meth:`COp.c_code` and other related ``c_**`` methods. Note that an | |||
| :class:`Op` can provide both Python and C implementations. | |||
|
|
|||
| :meth:`Op.make_thunk` method is another alternative to :meth:`Op.perform`. | |||
There was a problem hiding this comment.
This is outdated, and tbh I don't think it's necessary for 99.9999% of the people reading this page
ricardoV94
force-pushed
the
creating_an_op
branch
from
2f1a456 to
3a77d6b
Compare
ricardoV94
force-pushed
the
creating_an_op
branch
4 times, most recently
from
d37103d to
1d9bc2b
Compare
doc/extending/creating_an_op.rst
Outdated
| @@ -1,40 +1,18 @@ | |||
|
|
|||
| .. _creating_an_op: | |||
|
|
|||
| Creating a new :class:`Op`: Python implementation | |||
| Creating a new :ref:`op`: Python implementation | |||
There was a problem hiding this comment.
Where do you want the :ref:`op`/:class:`Op` to point to? To https://pytensor--1086.org.readthedocs.build/en/1086/extending/op.html or to https://pytensor--1086.org.readthedocs.build/en/1086/extending/graphstructures.html#op?
I think many of the broken cross-references come from this page: https://pytensor--1086.org.readthedocs.build/en/1086/extending/op.html, which is wildly inconsistent with itself. If the cross-reference would ideally point there we should fix this page first.
There was a problem hiding this comment.
Will check. I just wanted it to point somewhere, it wasn't before
There was a problem hiding this comment.
@OriolAbril extending op seems much more useful than the graphstructures#op, so I guess there.
doc/extending/creating_an_op.rst
Outdated
| has no bugs, and potentially profits from rewrites that have already been | ||
| implemented. | ||
| Odds are your function that uses existing PyTensor expressions is short, has no bugs, can be differentiated, | ||
| and may even work in non-default backends and benefit from optimization rewrites. |
There was a problem hiding this comment.
Suggestion: "A PyTensor function that builds upon existing expressions tends to be concise, reliable, and automatically differentiable, while often working seamlessly across different backends, thereby benefiting from optimization."
There was a problem hiding this comment.
Rewrote to something in that vein but simpler
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
ricardoV94
force-pushed
the
creating_an_op
branch
2 times, most recently
from
ec648b9 to
ade5653
Compare
ricardoV94
force-pushed
the
creating_an_op
branch
from
ade5653 to
f797c62
Compare
Changes: 1. Remove references to c-code which apply to `COp` but not `Op` 2. Fix failing doctests 3. Improve explanation of `make_node` 4. Emphasize distinction between itypes/otypes and make-node 5. Show `L_op` instead of `grad` 6. Show how to test `L_op` and `infer_shape` implementation 7. Simplify explanation of `__props__` and illustrate in example. 8. Introduce more complex multi-output Op to drive these details home 9. Remove old references to numba/ random variable Ops
ricardoV94
force-pushed
the
creating_an_op
branch
from
f797c62 to
c64736a
Compare
ricardoV94
changed the title
|
|
||
| - it first checks that the input :class:`Variable`\s types are compatible | ||
| with the current :class:`Op`. If the :class:`Op` cannot be applied on the provided | ||
| input types, it must raises an exception (such as :class:`TypeError`). |
| pass | ||
|
|
||
| def R_op(self, inputs, eval_points): | ||
| # Defines that symbolic expression for the R-operator based on the input variables |
|
|
||
| def R_op(self, inputs, eval_points): | ||
| # R_op can receive None as eval_points. | ||
| # That mean there is no diferientiable path through that input |
There was a problem hiding this comment.
2 Typos: "That means" and "differentiable"
|
|
||
| def perform(self, node, inputs, output_storage): | ||
| # Validate input type | ||
| if not x.type.ndim == 2 and x.type.dtype == "float64": |
There was a problem hiding this comment.
need to enclose conditions in parentheses
|
|
||
| x = pytensor.tensor.matrix() | ||
| x = matrix(x, dtype="float64") |
There was a problem hiding this comment.
matrix name should be enclosed in quotes
|
|
||
| For instance, to verify the :meth:`Rop` method of the ``DoubleOp``, you can use this: | ||
| For instance, to verify the :meth:`R_op` method of the ``DoubleOp``, you can use this: |
There was a problem hiding this comment.
do you mean "of the DoubleROp" here?
|
|
||
| .. code-block:: none | ||
| Finally we should test the gradient implementation. | ||
| For this we can use the ``pytensor.gradient.verify_grad`` utility which will compare the output of a gradient function with fininte differences. |
|
|
||
| PyTensor has some functionalities to test for a correct implementation of an :class:`Op` and it's many methods. | ||
|
|
||
| We have already seen some user facing helpers, but there are also test classes for :class:`Op` implementations |
See commit message for list of changes.
The changes are based on feedback from helping other devs and users
Tagging @OriolAbril because I'm 100% sure I messed up some sphinx stuff (sorry)
📚 Documentation preview 📚: https://pytensor--1086.org.readthedocs.build/en/1086/