Add storage support for RangeParameter.step_size (JSON + SQA)#5221
Open
saitcakmak wants to merge 2 commits into
Open
Add storage support for RangeParameter.step_size (JSON + SQA)#5221saitcakmak wants to merge 2 commits into
saitcakmak wants to merge 2 commits into
Conversation
Summary: TL;DR: Adds `step_size`, which is already exposed in ax/api `RangeParameterConfig` but resolves to `ChoiceParameter`, to `RangeParameter`. This will generalize and replace `digits` as a way to allow large discrete range parameters over a grid of possible values, with the downstream code deciding whether to treat it as a continuous or discrete parameter for optimization, based on parameter cardinality. Adds a `step_size` arg to `RangeParameter` that snaps values to a grid anchored at `lower` (in `cast()`), for both FLOAT and INT parameters. This is the first diff in the step_size unification stack: `step_size` will subsume both the discrete-grid and limited-resolution (`digits`) use cases under one knob. - Next diff will add storage support. The internal DB has already been updated to include the new column. - We will then migrate all current usage off `digits` and onto `step_size`. - We will add support for treating low-cardinality float-range parameters as discrete in `Adapter`, so that it is efficiently optimized over the correct grid (rather than having to use continuous optimization + rounding). - At this point, we will have proper support for `step_size`, so we can update the ax/api usage to leverage it, rather than resolving to `ChoiceParameter`. - We can then deprecate `digits` and do any remaining clean-up. In this diff `step_size` coexists with the existing `digits` arg (they are mutually exclusive at construction). Subsequent diffs in the stack migrate storage (JSON + SQA), transforms and utils, and the public API (`RangeParameterConfig`) to `step_size`, then deprecate `digits` in favor of it. Behavior: - `cast()` rounds `(value - lower) / step_size` to the nearest integer and returns `lower + n * step_size`. It does NOT clamp to `[lower, upper]`: an out-of-bounds input (e.g. a historical observation recorded outside the current bounds) snaps to the nearest grid point, which may itself be out of bounds. This mirrors the non-`step_size` `cast()`, which leaves out-of-bounds values in place rather than silently moving them into range — range validity is enforced by `validate()`, not `cast()`. - Both bounds must lie on the grid: `(upper - lower)` must be an integer multiple of `step_size` (within `EPS`). Off-grid bounds are rejected at construction. This guarantees `upper` is itself a feasible value, so a value near the upper bound snaps to `upper` rather than to a grid point short of it. - `step_size` must be strictly positive, and must be integer-valued for INT parameters. - `cardinality()` accounts for `step_size`: a grid-valued FLOAT reports the finite number of grid points instead of `inf`, and a grid-valued INT counts grid points rather than every integer in `[lower, upper]`. `step_size` defines a discrete grid but does not, by itself, force discrete acquisition optimization; how the optimizer treats the parameter depends on the grid cardinality and is determined at the generator level. Differential Revision: D107274057
Summary: Teaches both storage backends to persist and restore the new `step_size` field on `RangeParameter`. Second diff in the step_size unification stack (builds on the native `step_size` support added in D107274057). The underlying SQA column (`SQAParameter.step_size`) and the corresponding EntAE schema were added and landed separately (D107108021, D107112306), so this diff is pure encoder/decoder logic — it does not touch the schema. SQA: the encoder writes `parameter.step_size` to the column (and continues writing the legacy `digits` column for now; `digits` is dropped in a later diff). The decoder prefers `step_size` and falls back to the legacy `digits` column for rows written by older code, passing only one to the constructor (which rejects both being set; it converts `digits` to `step_size` internally). JSON: `range_parameter_to_dict` emits `step_size` alongside `digits`. There is no custom JSON decoder for `RangeParameter` — the registry maps it directly to the class, and `ax_class_from_json_dict` splats the serialized dict straight into the constructor. So `step_size` flows through automatically, and legacy blobs carrying only `digits` still decode via the constructor's back-compat handling. No decoder change is required for JSON. Differential Revision: D107282941
meta-cla
Bot
added
the
CLA Signed
|
@saitcakmak has exported this pull request. If you are a Meta employee, you can view the originating Diff in D107282941. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5221 +/- ##
========================================
Coverage 96.50% 96.51%
========================================
Files 617 617
Lines 69776 69913 +137
========================================
+ Hits 67339 67476 +137
Misses 2437 2437 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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:
Teaches both storage backends to persist and restore the new
step_sizefield onRangeParameter. Second diff in the step_size unification stack (builds on the nativestep_sizesupport added in D107274057).The underlying SQA column (
SQAParameter.step_size) and the corresponding EntAE schema were added and landed separately (D107108021, D107112306), so this diff is pure encoder/decoder logic — it does not touch the schema.SQA: the encoder writes
parameter.step_sizeto the column (and continues writing the legacydigitscolumn for now;digitsis dropped in a later diff). The decoder prefersstep_sizeand falls back to the legacydigitscolumn for rows written by older code, passing only one to the constructor (which rejects both being set; it convertsdigitstostep_sizeinternally).JSON:
range_parameter_to_dictemitsstep_sizealongsidedigits. There is no custom JSON decoder forRangeParameter— the registry maps it directly to the class, andax_class_from_json_dictsplats the serialized dict straight into the constructor. Sostep_sizeflows through automatically, and legacy blobs carrying onlydigitsstill decode via the constructor's back-compat handling. No decoder change is required for JSON.Differential Revision: D107282941