payload config: add new Probability<const u32: MIN_AS_BITS> type#1874
Merged
Conversation
goxberry
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 979097bcb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Error returned when a value cannot be turned into a [`Probability`]. | ||
| #[derive(Debug, thiserror::Error, Clone, Copy)] | ||
| pub enum ProbabilityError { | ||
| /// Value is NaN or `±∞`. |
There was a problem hiding this comment.
/workspace/lading/AGENTS.md requires "US-ASCII only - no Unicode characters in code or documentation", but this addition introduces non-ASCII glyphs here and in the new plan docs (for example ±∞, ↔, em dashes, and ∈). Since the rule applies to both Rust documentation and Markdown, replace these with ASCII equivalents such as +/-inf, <->, --, and in before merging.
Useful? React with 👍 / 👎.
blt
reviewed
May 12, 2026
Comment on lines
+96
to
+100
| Approximately 18 unit tests collapse into 12 generic property tests | ||
| (of which the `MIN`-parameterized ones expand to three `#[test]` functions | ||
| each) plus 6 retained unit tests. Input-space coverage broadens substantially | ||
| while the literal wire formats and discrete edge cases (`-0.0`, `[1.0, 1.0]`) | ||
| remain pinned by unit tests. |
blt
approved these changes
May 12, 2026
This comment has been minimized.
This comment has been minimized.
Introduces an f32-valued probability type in lading_payload::common::config whose lower bound is carried as a const-generic u32 (an IEEE-754 bit pattern obtained at the call site via f32::to_bits). The decoded bound is validated at compile time via assertions in `Self::MIN` against the constraints in docs/plans/probability-type.md: finite, in [+0.0, +1.0], not -0.0. Runtime construction goes through `try_new`, which rejects NaN, ±∞, -0.0, values below `MIN`, and values above 1.0 with a typed `ProbabilityError`. `Self::MIN` is referenced inside the constructor so that the const-eval checks fire for every monomorphization. Serde wiring and a Display impl follow in subsequent commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires Probability<MIN_BITS> into Serde with the canonical newtype-validation pattern: `#[serde(into = "f32", try_from = "f32")]` together with `TryFrom<f32>` (delegating to `try_new`) and `From<Probability<_>> for f32`. On the wire the type is a bare f32, so existing YAML/JSON configuration that currently uses an unvalidated f32 field can be migrated to this type without breaking deserialization. Invalid values are rejected at parse time with the same ProbabilityError variants surfaced through serde's error path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Delegates to the inner f32's Display so format specifiers like `{:.3}`
propagate. Mirrors the existing ConfRange<T>: Display impl in the same
module, which similarly does not surface its type parameter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Document why MIN_BITS is a u32 (stable Rust lacks f32 const generics) and the IEEE-754 properties of the f32 <-> u32 round trip that make integer- domain bound checks viable on non-negative finite values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename MIN_BITS to MIN_AS_BITS and NEG_ZERO_BITS to NEG_ZERO_AS_BITS to make the f32-encoded-as-u32 representation explicit at every use site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace per-case unit tests for `Probability<MIN_AS_BITS>` with proptest properties covering value validation, Display, and serde (JSON + YAML) round-trips and rejections. Each MIN-parameterized property is realized as a small generic helper invoked from one `proptest!` block per type alias (ZeroOrMore, AtLeastHalf, AtLeastOne). Kept as unit tests where exact wire format or a discrete edge case matters: the const decoding pins, the `[1.0, 1.0]` unit-bound case, the `-0.0` rejection, and the JSON/YAML wire-format pins for `0.75`. `valid_value_strategy` is boxed and branches on `min >= 1.0` because proptest's `RangeInclusive<f32>` strategy panics on degenerate ranges. The `non_finite_strategy` uses only `Just(NAN)`, `Just(INFINITY)`, and `Just(NEG_INFINITY)` rather than additionally filtering `any::<f32>()` as the plan text suggests — the filter would exhaust proptest's rejection budget, and the three discrete values already exercise every branch of the `is_finite()` rejection path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove the `ignore` fence on the Probability example so it compiles and runs as part of `cargo test --doc`. Since 0.75 is exactly representable in f32, tighten the assertion to an equality check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the explicit `-0.0` rejection with normalization to `+0.0` so configs containing `-0.0` no longer fail validation. The order-isomorphism between `f32` and `u32` bit patterns that the type relies on is preserved because stored values are never `-0.0`. Drop the `ProbabilityError::NegativeZero` variant. BREAKING CHANGE: `ProbabilityError::NegativeZero` is removed. Callers matching on it should drop the arm; `-0.0` now succeeds (for `MIN <= 0.0`) or surfaces as `BelowMin` with `value: +0.0` (for `MIN > 0.0`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All operations in `try_new` (Self::MIN, f32::is_finite, f32::to_bits, f32 comparisons, enum/struct construction) are const-stable on the pinned Rust 1.90 toolchain, so the function can be marked `const`. This lets callers validate `Probability` literals at compile time by matching on the returned `Result`. Add `try_new_is_const_evaluable` to lock in the property: it materializes both a successful and a failing `try_new` call into `const` bindings, so losing `const` would break the build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The type doc previously hinted that bound checks might one day migrate from the f32 domain to u32 comparisons on `value.to_bits()`. In practice that change would add branches (to handle negative inputs without mis-classifying them as `AboveOne`) and saves no measurable time on a non-hot path, so the promise was misleading. Recast the IEEE-754 properties paragraph so the bullets stand on their own as design rationale for the `-0.0` normalization and the bit-pattern `Hash` impl, rather than as setup for a future integer-domain comparison. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace `±∞`, `+∞`, and `↔` with ASCII equivalents (`<->`, `+inf`) and intra-doc links to `f32::NAN`, `f32::INFINITY`, and `f32::NEG_INFINITY` for portability and better rustdoc cross-referencing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Explain that the const generic parameter exists to support fields with a strict positive lower bound, citing `unique_tag_probability` in `common::tags::Generator` as the motivating use case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
goxberry
force-pushed
the
goxberry/create-probability-type
branch
from
c63855a to
febc1e4
Compare
Contributor
Author
|
Will submit a follow-on PR with a few more ergonomics tweaks, and wire up payload configs to see whether this experiment proves out. |
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.
What does this PR do?
Adds a new
Probability<const u32: MIN_AS_BITS>type as a proof-of-concept for configuration fields that acceptf32values interpreted as probabilities. The type isn't used yet. TheMIN_AS_BITSfield is intended for use as a lower bound in cases where the minimum probability for a configuration field should be positive, e.g., unique tag probability of at least 0.1.Motivation
Reduce some boilerplate validation code related to probability-valued configuration fields for payloads.
Related issues
n/a
Additional Notes
Tests and ergonomics are still a work in progress. The plan files will be removed prior to merging the PR. These files are present in case reviewers find them helpful, but I don't intend them to clutter the repo.