Conversation
`quantecon.random.draw` chose between an array of draws and a single scalar draw with `isinstance(size, int)` in the Python body and `isinstance(size, types.Integer)` in the `@overload`. A NumPy integer is not a Python `int` but is a `types.Integer`, so `draw(cdf, np.int64(10))` returned one scalar from Python and ten draws from a jitted caller, with no error either way. `bool` is the reverse case: it passes `isinstance(x, int)` but Numba types it as `Boolean`. Widening the Python predicate to `(int, np.integer)` and excluding `bool` makes the two paths agree on every input in #918's table, verified including `None` across all four call shapes. The `@overload` needs no change: `types.Boolean` is not a subclass of `types.Integer`, so the jitted path already dispatches every case correctly. Behaviour change: `draw(cdf, np.int64(10))` now returns ten draws from Python where it previously returned one scalar. That is what the caller asked for and what jitted callers already received, but it is silent and needs a line in the 0.12.0 release notes. Closes #918. Replaces #919. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
A regression test for the size=True / np.bool_(True) scalar-dispatch case is still missing even though the change explicitly addresses that edge case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an inconsistency in quantecon.random.draw where the pure-Python implementation treated size=np.int64(10) as a scalar draw, while the Numba-jitted overload treated it as an array-sized draw, leading to divergent return types depending on whether the caller was jit-compiled.
Changes:
- Update the pure-Python
drawsize-dispatch predicate to accept NumPy integer scalars (and explicitly excludebool) so it matches the@overloaddispatch behavior. - Add a regression test asserting NumPy-integer
sizerequests an array return for both Python and jitted call paths.
File summaries
| File | Description |
|---|---|
quantecon/random/utilities.py |
Align pure-Python size dispatch with the Numba overload by accepting np.integer (excluding bool). |
quantecon/random/tests/test_utilities.py |
Add regression coverage ensuring NumPy integer size returns an array on both call paths. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def test_numpy_integer_size(self): | ||
| """ | ||
| A numpy integer `size` must request an array, as a Python `int` | ||
| does and as the jitted path already did. See #918. | ||
|
|
||
| """ | ||
| size = np.int64(10) | ||
| for func in self.draw_funcs: | ||
| out = func(self.cdf, size) | ||
| assert_(out.shape == (size,)) | ||
|
|
|
coverage: 90.884%. remained the same — fix/draw-numpy-integer-size-918 into main |
oyamad
left a comment
oyamad
left a comment
There was a problem hiding this comment.
Here's my thought: I wouldn't make (the Python version of) draw(cdf, size=True) return any value. As I stated in #917 (comment), we should only fix the bug I had introduced, keeping the behavior of the Python version "unspecified" or "unsupported" (the @overload is the contract). And thus I wouldn't add any new test.
| # `bool` subclasses `int` in Python but Numba types it as `Boolean`, | ||
| # not `Integer`, so the exclusion keeps this branch in step with the | ||
| # `@overload` implementation below. See #918. | ||
| if isinstance(size, (int, np.integer)) and not isinstance(size, bool): |
There was a problem hiding this comment.
| if isinstance(size, (int, np.integer)) and not isinstance(size, bool): | |
| if isinstance(size, (int, np.integer)): |
Aligns the Python
sizedispatch inquantecon.random.drawwith the jitted path. Replaces #919, which was closed with thanks; this is the reduced form of the same fix.The bug
drawdecides between "an array ofsizedraws" and "one scalar draw" in two places, with two tests that do not agree:isinstance(size, int)@overloadimplementationisinstance(size, types.Integer)A NumPy integer is not a Python
intbut is atypes.Integer, so the same call returns different things depending on whether the call site is jit-compiled — and nothing warns. This is easy to hit:sizearrives asnp.int64from any ordinary numpy expression (counts[i],arr.sum(),arr.argmax(), a//on an array), whilelen(x)andarr.shape[0]give Python ints and are fine, which is why it survives casual testing.The change
One line in the Python body, plus the comment explaining it:
The
boolexclusion is required in the other direction:boolsubclassesintin Python, but Numba types it asBoolean, so without it the two paths would still disagree forsize=True.The
@overloadneeds no change.types.Booleanis not a subclass oftypes.Integer, so the jitted path already dispatches every case correctly — only the Python body was wrong.Verified
Dispatch table on this branch, Python against jitted, for every case in #918:
size10np.int64(10)np.int32(10)np.uint8(10)Truenp.bool_(True)10.0NoneNonewas checked across all four call shapes — omitted and explicit, from Python and from a jitted caller — since_is_no_rngshows Numba spells "no value" more than one way.flake8 --select=F401,F405,E231 quantecon: cleangit diff --check: cleanRelease note needed
draw(cdf, np.int64(10))now returns ten draws from Python where it previously returned one scalar. That is what the caller asked for, and what jitted callers already received, but it is a silent behaviour change and should be called out in the 0.12.0 release notes. Per AGENTS.md there is no per-PR changelog entry to add.Closes #918. Replaces #919.
🤖 Generated with Claude Code