Fix stale yw_to_w / element_wise fn references in doc notebooks#136
Merged
Conversation
braintrace's ETP rule naming was renamed YW_TO_W -> DT_TO_T in PR #130, but three notebooks (etp_primitives.ipynb, customizing_primitive_transforms.ipynb, limitations.ipynb) still referenced the old names; two executable cells in etp_primitives.ipynb raised ImportError/TypeError if re-run. While verifying by re-executing the notebooks, also found and fixed stale element_wise(weight, fn=...) calls (renamed to weight_fn) in etp_primitives.ipynb and docs/quickstart/concepts.ipynb. All four notebooks were re-executed end-to-end to confirm they run cleanly.
Reviewer's GuideDocumentation maintenance PR that updates tutorial/advanced notebooks to match the current ETP rule API ( File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Several notebooks now include machine-specific stderr output (e.g., CUDA and compiler warnings) and execution metadata; consider clearing outputs before committing to keep docs clean and reproducible across environments.
- In
etp_primitives.ipynbthe summary still states "8 built-in primitives" but the listed primitives now include additional ones (e.g.,etp_einsum,etp_emb,etp_gmm); update the count or wording to match the current set. - In
docs/quickstart/concepts.ipynbthe gradient computation block appears twice with identical code; you can remove the duplicate cell to avoid confusion for readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several notebooks now include machine-specific stderr output (e.g., CUDA and compiler warnings) and execution metadata; consider clearing outputs before committing to keep docs clean and reproducible across environments.
- In `etp_primitives.ipynb` the summary still states "8 built-in primitives" but the listed primitives now include additional ones (e.g., `etp_einsum`, `etp_emb`, `etp_gmm`); update the count or wording to match the current set.
- In `docs/quickstart/concepts.ipynb` the gradient computation block appears twice with identical code; you can remove the duplicate cell to avoid confusion for readers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
etp_primitives.ipynb,customizing_primitive_transforms.ipynb,limitations.ipynb) still referenced braintrace's pre-Rename YW_TO_W ETP rule to DT_TO_T #130yw_to_w/ETP_RULES_YW_TO_Wrule naming instead of the currentdt_to_t/ETP_RULES_DT_TO_T. Two executable cells inetp_primitives.ipynbraisedImportError/TypeErrorif re-run.element_wise(weight, fn=...)calls (parameter renamed toweight_fn) inetp_primitives.ipynbanddocs/quickstart/concepts.ipynb.## UNRELEASED.Test plan
python -m mypy braintrace— no issues foundpython -m pytest braintrace/ -q— 2123 passed, 0 failed, 3 skippedjupyter nbconvert --to notebook --execute --inplace— all run cleanly with no errorsgrep -rn "yw_to_w\|YW_TO_W"across the repo — zero remaining matches outside historicalchangelog.mdentriesSummary by Sourcery
Update documentation notebooks to match current BrainTrace ETP rule and primitive APIs and ensure all examples execute cleanly.
Documentation:
yw_to_w/ETP_RULES_YW_TO_Wtodt_to_t/ETP_RULES_DT_TO_T.braintrace.element_wiseto use the renamedweight_fnparameter in tutorials and quickstart concepts.Chores: