Repository navigation
fix: qrandint/qlograndint never sample the documented inclusive upper bound - #1606
Li Jiang (thinkall) merged 13 commits into
Conversation
… bound Quantized.sample()'s q=1 fast path skips quantization and calls the wrapped Integer sampler directly, which draws exclusive of domain.upper, same as plain randint. Since qrandint defaults to q=1, its own "upper is also inclusive (!)" docstring was never true. For q>1 the general branch rounds a raw exclusive-upper draw, so reaching the nominal upper depends on the rounding fraction. Fixed by extending the sampled range by one quantization step for Integer domains before drawing, then clamping back to the true upper bound. Plain randint/lograndint never go through Quantized, so they are untouched. Fixes microsoft#1605
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Two correctness issues remain:
flaml/tune/sample.py:459: routing the defaultq=1path through float quantization changes batched samples from integer NumPy arrays to lists ofnp.float64, and can lose precision above2**53. Preserve an integer-onlyq == 1path and add scalar/batched type and large-bound tests.flaml/tune/sample.py:470-474: extending the sampler byqdraws values outside the requested domain and clamps them into the upper endpoint, biasing its probability (also in log space). Extend an integer sampler's exclusive upper bound by one, or sample quantization-grid indices directly, and add deterministic edge-bin/distribution tests.
The q=1 fast path for Integer domains routed through float division, turning integer samples into np.float64 and losing precision above 2**53. For q>1, extending the raw draw's range by a full q and clamping the overshoot into the top bin gave that bin roughly 1.4x the width of an interior bin (measured with a fixed seed: 40164 vs ~39800 draws per bin over 200000 samples on a 5-bin grid, expected 40000 each). Draw the grid point's index directly instead: every point, including the top one, is reachable through exactly one raw index, so no bin is over- or under-represented and the result stays a plain int with no rounding. Added scalar/batched dtype tests, a large-bound precision test past 2**53, and a fixed-seed bin distribution test that would have caught the top-bin bias.
|
Both real. Pushed fd63fcd: instead of rounding a raw draw and clamping, it now draws the quantization grid point's index directly, so the q=1 default stays a plain int (no float division) and every grid point including the top one gets exactly one raw index instead of the top bin absorbing the clamped overshoot. Verified the bias with a fixed seed: the old code put 40164 of 200000 draws in the top bin of a 5-bin grid against ~39800 for the others, expected 40000 each. New code comes back within 0.5% on every bin. Added that as a deterministic test, plus scalar/batched dtype checks and a large-bound (2**60) precision test. Ran the full test/tune/test_sample.py suite locally, 9 passed. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
The new commit fixes the previous out-of-domain endpoint clamping, but three blockers remain:
flaml/tune/sample.py:469-478: computing grid bounds withdomain.lower / self.qconverts integers to float. For nonzero bounds above2**53, scalar sampling can return a value below the requested domain and batched sampling can overflow on Windows. Calculate ceiling/floor grid indices using integer division and keep offset arithmetic in Python/safe integer types; add scalar and batched large nonzero-bound tests.flaml/tune/sample.py:478: theq=1batched compatibility issue remains. Historicallysize > 1returned an integer NumPy array; the new path returns a Python list. Preserve the ndarray shape/dtype and assert the container and dtype in the regression test.flaml/tune/sample.py:472-478: rebasing the grid to indices starting at1changesqlograndintprobabilities wheneverlower > q. For example,qlograndint(10, 20, 1)samples log-uniformly over1..11and shifts by nine instead of sampling over the actual10..20values. Sample absolute grid indices (or weight the actual grid values) and add a deterministic distribution test withlower > q.
…nteger arithmetic Addresses thinkall's second review round on microsoft#1606: qlograndint drew log- uniformly over an index range rebased to start at 1, which is a different, shifted distribution from the actual value grid whenever lower > q (log- uniform is scale-invariant under multiplication, not under an additive shift). Switched to the grid's own index (value // q). Also replaced the remaining float division for computing grid bounds with integer ceiling/ floor division, and the batched Integer path now returns an int64 ndarray instead of a Python list, matching the historical container and dtype.
|
Pushed b7d9bfb. 1/2: replaced the float division with integer ceiling/floor division for the grid bounds, and the batched path now returns an int64 ndarray (matches test_qrandint_returns_plain_int_scalar_and_batched and a new test_qrandint_large_nonzero_lower_bound_batched). 3: sampled the grid's own index (value // q) instead of one rebased to start at 1. LogUniform is scale-invariant under multiplication but not under an additive shift, so the rebase was the actual bug. Added test_qlograndint_stays_log_uniform_when_lower_much_greater_than_q, which compares the empirical mean of qlograndint(1000, 1010, 1) against the analytic LogUniform(1000, 1010) mean. Checked it discriminates: reverting just the sample.py change fails that test with a 2.0 gap against a 0.1 tolerance (the rebased implementation's mean sits about 200 standard errors off). Full suite (11 tests) passes in the repo's own Docker command. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
The latest commit resolves the large-integer precision issue, restores the q=1 batched ndarray contract, and samples qlograndint from absolute grid values. One compatibility blocker remains:
flaml/tune/sample.py:484: the unconditional ndarray return also changes batchedqrandint/qlograndintwithq > 1from their historical Python list return tonp.ndarray. Preserve the exact integer-grid calculation, but return an ndarray only forq == 1and retain the list container forq > 1. Updatetest/tune/test_sample.py:100-102to assert the two distinct historical contracts rather than codifying the broader return-type change.
…ive-upper-bound # Conflicts: # flaml/tune/sample.py
|
Rebased onto main, it conflicted with #1599 on the same line. Both PRs were carving out opposite exceptions to the same q==1 fast path (yours excluded Float, mine excluded Integer), so the correct merge removes the fast path for both types and always routes through quantization. Pushed 43f1799; all 29 tests pass locally including your new test_unit_quantization.py. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
The main-branch integrations preserve the inclusive-bound, integer-precision, and log-space fixes, but the remaining compatibility blocker is unchanged:
flaml/tune/sample.py:481: batched integer quantization unconditionally returnsnp.ndarray. Historicallyq == 1returned an integer ndarray, whileq > 1returned a Python list throughlist(quantized). Preserve the ndarray only forq == 1, retain the list contract forq > 1, and updatetest/tune/test_sample.py:101so it does not codify the regression.
Quantized.sample()'s Integer branch unconditionally returned an ndarray for size > 1, but only q == 1 historically did; q > 1 returned a plain Python list (matching the float quantization path's list(quantized)). Return a list for q > 1 and keep the ndarray only for q == 1. Updated the two tests that asserted ndarray for a q > 1 domain so they check the correct historical container instead.
|
Right, that unification was the actual gap. Pushed 8c4a8c0: q == 1 still returns the ndarray, q > 1 now returns a plain list via list(values), and the two tests that had assumed ndarray for a q > 1 domain now check the list container instead. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
The q > 1 list contract is restored, but one q == 1 compatibility issue remains:
flaml/tune/sample.py:476-486: convertingindicestonp.int64before theq == 1branch changes the wrapped sampler's historical ndarray dtype. On Windows, the priorqrandint(..., size>1)path returnedint32; the current head forcesint64. Returnindicesdirectly for batchedq == 1, and perform theint64conversion/multiplication only for theq > 1list path. Add a regression assertion matching the underlying sampler's dtype.
|
Right, forcing int64 there was unnecessary. Pushed 2ecd58a: q == 1 now returns |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
The previous q == 1 dtype blocker is resolved: returning indices preserves the wrapped sampler's platform-specific integer ndarray dtype. One large-domain blocker remains:
flaml/tune/sample.py:487: batchedq > 1converts indices tonp.int64before multiplication. For grids above2**63 - 1, multiplication wraps and can return zero or negative values outside the requested domain, while scalar sampling remains correct. Construct the historical list by multiplying each index as a Pythonint, and extend the large batchedqrandint/qlograndinttests beyond signed 64-bit range.
Grid points above 2**63 - 1 wrapped silently through numpy int64 multiplication in the batched q > 1 Integer path; multiply each index as a Python int instead.
|
Batched q > 1 now multiplies each grid index as a Python int instead of casting through np.int64, so it no longer wraps above 263 - 1. Added test_qrandint_batched_beyond_int64_range (q=262, upper=2**65); against the prior code it fails with a negative wrapped value, e.g. -4611686018427387904. Full 13-test suite plus ruff check pass in the repo's own Docker verify command. Pushed 6142c34. |
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review. Batched q > 1 multiplication now stays in Python integers, preventing signed-64-bit overflow while preserving the historical contracts: scalar Python integers, q == 1 ndarray with the underlying platform dtype, and q > 1 Python lists. Inclusive bounds, negative/non-divisible domains, large-integer precision, and actual-grid qlograndint behavior are covered and pass. No blocking issues found.
There was a problem hiding this comment.
🟡 Changes recommended
The implementation changes the public custom-sampler contract for integer quantized domains and can produce incorrect results.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes inclusive upper-bound sampling for quantized integer samplers while preserving plain integer sampler behavior.
Changes:
- Updates integer quantized sampling and upper-bound handling.
- Adds regression coverage for bounds, precision, batching, and distributions.
- Preserves float quantization behavior.
File summaries
| File | Summary |
|---|---|
test/tune/test_sample.py |
Adds comprehensive regression tests. |
flaml/tune/sample.py |
Updates integer quantized sampling. |
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.
The prior merge from main flattened this file's CRLF line endings to LF, so the PR diff showed the whole file as removed and re-added instead of the actual ~30-line change.
|
Good catch, that was a real problem. A merge from main earlier in this branch's history flattened this file's line endings from CRLF to LF, so the diff showed the whole file as rewritten. Converted it back to CRLF and pushed; the diff is now the actual ~30-line change plus the new test file. |
There was a problem hiding this comment.
🟡 Changes recommended
Correct the biased log-uniform expected-value assertion in test/tune/test_sample.py.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Li Jiang (thinkall)
left a comment
There was a problem hiding this comment.
Reviewed the updates since my prior approval. The author commit changes only line endings, and the merge from main does not touch the sampling implementation or tests. The approved scalar/container/dtype contracts, large-integer Python arithmetic, inclusive bounds, precision, and log-grid behavior remain intact. No blocking issues found.
There was a problem hiding this comment.
🟡 Changes recommended
Non-divisible upper bounds remain unreachable despite the documented inclusive contract.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
test/tune/test_sample.py:208
- This repeats both domain construction and scalar log-sampling 200,000 times. A single batched sample exercises the same distribution while avoiding hundreds of thousands of Python calls and object copies.
samples = [qlograndint(lower, upper, q).sample(spec=None, random_state=rs) for _ in range(n)]
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
…lt-in samplers A custom sampler on an Integer domain got the grid-index domain (and, for q > 1, its output multiplied by q) unconditionally, which breaks any sampler that reads domain.lower/domain.upper or returns actual values rather than an index. Only Integer._Uniform and Integer._LogUniform use the grid transform now; every other sampler keeps the historical actual-value contract, q == 1 calling it directly and q > 1 falling through to the existing float-based quantization.
|
Applied the suggested fix as given: Added the two regression tests: a custom subclass of |
The custom-sampler-contract fix (6320d97) widened the q == 1 direct passthrough from Integer-only to isinstance(domain, Integer) == False domains too, so a Float domain (quniform/qloguniform/qrandn) at q == 1 skipped the round-to-nearest-unit step entirely and returned the raw unrounded sample. Scope the passthrough back to Integer domains, which is what it was gating before this round's change. test/tune/test_unit_quantization.py::test_unit_quantization_rounds_float_samples covers this; it fails on the current head (6 of 18 parametrizations, all q=1) and passes with this fix.
|
CI caught a regression from the last push: the q == 1 passthrough I widened to gate on Verified: reverted only sample.py and reran the failing parametrizations, all 6 fail with the exact unrounded-vs-rounded mismatch; with the fix, the full test_sample.py + test_unit_quantization.py suite (33 tests) passes in a clean container. Pushed c095728. |
|
Thanks for the thorough review rounds on this one, Li Jiang (@thinkall). The q==1/q>1 contract split was the part I kept getting wrong, and your catches on the int64 overflow and CRLF diff noise both landed real bugs. |
## Summary Merge current public main, retaining the collaborator's quantized-sampling update, the ordinal-encoder refactor, and the current Codecov action. Port the missing internal fANOVA replay expansion and fitted-model forecast frequency fix. Bring over notebook artifact isolation and related ensemble and resampler regression coverage without merging internal Git history. ## Prompting Intent The engineer requested another comparison and synchronization after new FLAML-Internal PRs merged. Keep GitHub as the maintained library source, preserve public installation and forecasting contracts, and exclude internal delivery files. After discussing smoothing initialization, the engineer requested the better general choice everywhere rather than Fabric-specific initialization behavior. ## Linked Sources - Consolidation PR: #1545 - Current public main: 16bc42e - Public categorical encoding update: #1569 - Public sampling update already on this branch: #1606 - Internal changes audited from the newly merged source; provenance remains in the internal repository - Runtime and synchronization decisions: flaml/fabric/README.md ## Rationale Widen replay distributions around observed trials without mutating the original search space or discarding compatible log scales and grids. Derive a regular forecast calendar from the model actually fitted to the CV fold, not from the full dataset's irregular-date fallback. Retain estimated smoothing initialization uniformly: it fits the initial state instead of locking it to the first observation. Keep the existing public DatetimeIndex/date-gap contract and package version, rather than importing internal observation-row output or release-only metadata. Changes already present in the public branch are not duplicated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0e4942d-f489-4b42-8a90-097ede4e88f7
Fixes #1605
Quantized.sample()'s q=1 fast path bypasses quantization and calls the wrappedIntegersampler directly, which draws exclusive ofdomain.upper(matching plainrandint's own exclusive-upper contract). Sinceqrandint's default is q=1, its documented inclusive upper bound was unreachable. For q>1 the general branch rounds a raw exclusive-upper draw, so whether the nominal upper is reached depends on the rounding fraction, e.g.qrandint(0, 10, 2)topped out at 8.Fixed by, for
Integerdomains only, extending the sampled range by one quantization step before drawing and clamping the result back to the true upper bound afterwards. Plainrandint/lograndintnever go throughQuantizedat all, so they're untouched, and I left the Float q=1 fast path alone since #1599 is already narrowing that one.One thing to flag: this touches the same
if self.q == 1:line as #1599. Whichever lands first, the other will need a small rebase.Added tests for qrandint at q=1 (default and explicit) and q=2, and qlograndint at q=1: none of them can reach the upper bound on main, all do here. Also a control test that plain randint keeps its exclusive upper. test/tune/test_constraints.py exercises qrandint inside a real BlendSearch run and still passes.