Skip to content

discrete_dp review overlay recommends qe.tic/qe.toc, contradicting qe-code-005 and pointing at a deprecated API #36

Description

@mmcky

One reviewer action in the discrete_dp overlay tells an author to adopt qe.tic/qe.toc. That is wrong twice over: it contradicts the rule it cites, and it points at the API that QuantEcon/QuantEcon.py#833 is deprecating.

The finding

reviews/lecture-python-advanced.myst/discrete_dp.json, actions[6]:

Sweep the remaining single-instance items: … swap the three %timeit calls at 844-846 for qe.tic/qe.toc (qe-code-005), …

That surfaces in the generated report at lectures/lecture-python-advanced.myst/discrete_dp.md:67.

It contradicts the rule it cites. lectures/spec.md:64 defines qe-code-005 as "Use quantecon.timeit for benchmarking", and tools/qestyle_rules.py:1310 emits the hit text "hand-rolled benchmark loop — use qe.timeit". The correct recommendation for three %timeit calls is qe.timeit, not the Matlab-like timers.

It points at a deprecated API. QuantEcon/QuantEcon.py#833 makes tic, tac, toc and loop_timer emit a DeprecationWarning — deprecated at 0.12.0, removal targeted at v1.0 — as step 1 of QuantEcon/QuantEcon.py#786. An author who followed this action today would write code that warns on every call, and would have to redo the work at step 2.

Suggested fix

Edit actions[6] in reviews/lecture-python-advanced.myst/discrete_dp.json to name qe.timeit, then regenerate the report with tools/qestyle_draft.py per UPDATE.md — the .md is generated, so it should not be hand-edited.

One thing to confirm rather than assume: the overlay carries "source": {"commit": "b83d6da…", "blob": "845cbae…"}. Correcting the prose does not change which blob was judged, so my reading of the provenance convention is that the stamp stays as it is and this is not a re-review. Worth a maintainer's confirmation before the edit lands, since qestyle_status.py joins on it to tell fresh overlays from stale ones.

Scope

This is the only instance. I swept the whole repo for qe.tic/qe.toc/tic/toc mentions; besides this overlay and its generated report, the only other hit is lectures/spec.md:63, which reads "Mechanical (detect time.time() / tic/toc)" — correct, since that describes what the detector looks for rather than what an author should write. tools/qestyle_rules.py:1264 and tools/qestyle_report.py:96 both prescribe qe.Timer correctly.

Context

Found while auditing whether QuantEcon/QuantEcon.py#833 would affect the lecture series. It does not — all nine repos in the lectures workspace manifest are clean of the deprecated timers, verified with three independent grep engines. This overlay and one note in QuantEcon/QuantEcon.manual (tracked at QuantEcon/QuantEcon.manual#151) were the only two places left in the org still steering authors to the old API. The canonical QuantEcon/style-guide rule qe-code-004 is already correct, listing qe.Timer() as ✅ and tic()/toc() as ❌.

🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething is wrong or broken

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions