Skip to content

Model OrdF64 as a non-NaN float#972

Open
Abeeujah wants to merge 6 commits into
rust-bitcoin:masterfrom
Abeeujah:ordf64-nonnan
Open

Model OrdF64 as a non-NaN float#972
Abeeujah wants to merge 6 commits into
rust-bitcoin:masterfrom
Abeeujah:ordf64-nonnan

Conversation

@Abeeujah
Copy link
Copy Markdown

@Abeeujah Abeeujah commented Jun 1, 2026

PositiveReal newtype for probability handling

Summary:
Replaces f64 and OrdF64 with PositiveReal newtype throughout the policy module, making invalid probability states unrepresentable at the type level.

Changes:

  1. Patch 1: Rename OrdF64 to PositiveReal.
  2. Patch 2: Extract PositiveReal into its own module.
  3. Patch 3: Implement (Add, Mul), display, and normalize for PositiveReal, ratio for Threshold.
  4. Patch 4: Add parse_probability helper.
  5. Patch 5: Use PositiveReal in the policy module.
  6. Patch 6: Fix lint warning.

closes #445

@Abeeujah Abeeujah force-pushed the ordf64-nonnan branch 2 times, most recently from 03a3a63 to fba0e68 Compare June 4, 2026 14:25
@apoelstra
Copy link
Copy Markdown
Member

We actually have a much stronger invariant available than "not NaN". It's actually true that our numbers are always positive and finite. So we can rename this type from OrdF64 to PositiveReal or even Probability.

Rather than making a ton of functions like insert_elem return errors, change them to take OrdF64s instead of f64s. Then we can implement Add/Mul (but not Div and Sub!) on OrdF64 and basically do everything in terms of these.

Similarly we should change cost_1d to take and return OrdF64s.

The only places that a NaN can actually occur are where we do division -- I believe these are all in best_compilations (which already returns a Result) and in threshold (which is only called by best_compilations. These divisions should be encapsulated:

  • the threshold one involves division by n which is guaranteed to be nonzero by the Threshold type, so we should refactor it to take a thresh rather than k and n separately
  • the other case is division by total where we're normalizing two values so they sum to one. We should encapsulate this into some sort of normalize function on OrdF64

@apoelstra
Copy link
Copy Markdown
Member

I would propose splitting this into several commits:

  • One renames the type from OrdF64 to PositiveReal (or PosReal or Positive if you don't like typing)
  • One moves the type from compiler.rs up to its own module in src/primitives
  • One adds Add, Mul impls to the type, a normalize method which takes two values and divides both by their sum, and a method on Threshold which returns k/n as a PositiveReal. None of these need to return a Result
  • One adds a parse_probability method to src/expression/mod.rs which wraps parse_num_nonzero but returns a PositiveReal instead of a u32
  • Then you can update the compiler to use all this new functionality, which will enforce the invariant without needing to change any compiler methods

Abeeujah added 6 commits June 6, 2026 12:15
Update the type name to better reflect its domain invariants. The
underlying floats are guaranteed to be both positive and finite,
making `PositiveReal` a more accurate and descriptive name than
`OrdF64`, which only highlighted the `Ord` trait implementation.
Add `Mul` and Add trait implementations for `PositiveReal` to enable
arithmetic operations on `PositiveReal` values. Display impl is needed
for `PositiveReal` to be used in `Policy`.

Provide a `normalize` method on `PositiveReal` to normalize two values
into a valid probability distribution.

Implement the `ratio` method on `Threshold` to expose the threshold
ratio (k / n) as a `PositiveReal`.
Introduce a `parse_probability` function to parse non-zero strings
into PositiveReal values.

This implementation wraps around the `parse_num_nonzero` function
to safely parse string inputs, enforcing the non-zero invariant.
Change `Or` weights from usize to PositiveReal and all probability
parameters from raw f64 to PositiveReal, making invalid states
unrepresentable. Thread Threshold through the compiler instead
of extracting k/n pairs.
`cargo +nightly rbmt lint` fails with useless use of `format!`, this
patch fixes the culprit and uses `to_string` over the previously used
`format!` macro.
@Abeeujah
Copy link
Copy Markdown
Author

Abeeujah commented Jun 6, 2026

@apoelstra Thank you so much for the detailed spec you provided, I believe this is ready for review, although you didn't mention, using parse_probability resulted into me updating the Policy::Or variant, if this isn't what you had in mind, I can revert.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Model OrdF64 as NonNan

2 participants