Model OrdF64 as a non-NaN float

[Abeeujah]

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

[apoelstra]

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]

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]

@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.

[apoelstra]

Thanks! A few comments that should probably be addressed in followups:

• (in 1f25a35) change the pub to pub(super) on the f64 in PositiveReal; the internals are only needed by this module and the adjacent threshold module
• (in 7ae706a) please add #[must_use] to PositiveReal::normalize; also I think the method should be called normalized though I'm not sure
• (in 7ae706a) the Threshold::ratio method I think should be called k_over_n rather than ratio.
• (in 5d83c23) rather than having parse_probability directly construct a PositiveReal, it should use try_from and unwrap -- or better, parse_num_nonzero should be refactored into a private method that returns a NonZeroU32, and we can add a From<NonZero<T>> where T: Into<u64> impl on PositiveReal. And then use from with no unwrap

[apoelstra]

Actually, given the quantity of these I think it'd be better to update the PR rather than doing a folowup.

[Abeeujah]

• From<NonZero> where T: Into

This was only made stable in 1.79.0, and to use a generic T, it has to implement ZeroablePrimitive which isn't available on stable rust. I did TryFrom<u32>, and mapping the error to IllegalZero .

Every other changes were addressed as stated.

[apoelstra]

In cfb0336:

You cannot have a new method that takes an arbitrary f64 without checking whether it's positive.

You can call it new_unchecked but by the end of the PR you should be able to delete it.

In 3b0e7f9:

You definitely should not be calling PositiveReal::new(f64::INFINITY) or PositiveReal::new(0.0). Neither zero nor infinity are positive reals.

To handle 0 you will need to add some sort of conditional_add method to PositiveReal which takes an Option and either adds a value or doesn't.

To handle infinity I suspect you want to just back up, call the type PositiveF64 instead of PositiveReal, and we can drop the invariant that it be finite.

[Abeeujah]

I don't think we can entirely eliminate a constructor for this type, for the new method check/validation, I took the assert! route as propagating errors would need it to be handled by the caller, and you specifically mentioned that none of the compiler methods needs to be modified to return a Result.

All cases where we were having Zero has been handled, the INFINITY extracted to a constant as f64 would.

[apoelstra]

I think I will need to take over this PR. I see no reason that PositiveReals should be constructed inside compiler.rs at all.

[apoelstra]

Yeah, I see the complexity here. There are a bunch of changes that need to happen, possibly at once (though I'll try to untangle them into intellegible commits) if we want to eliminate all the constructors in compiler.rs.

In particular, Policy::Or needs to stop using usize and start using PositiveF64, which affects the entire file (and then some).

[apoelstra]

Further complicating things is that the compiler sometimes uses 0 probablities to mean "unsatisfiable". But I think this is confused logic that comes from the compiler attempting to be overly generic over different fragments.

As always, as soon as I pull on one thing with this library it turns out there's a whole ediface of weird hacks that come tumbling down..

[Abeeujah]

Further complicating things is that the compiler sometimes uses 0 probablities to mean "unsatisfiable". But I think this is confused logic that comes from the compiler attempting to be overly generic over different fragments.

I think I handled the 0-probability case in 0243267 those were PositiveF64::new(0.0) placeholders, now replaced with Option + conditional_add.

The real blocker going forward for me isindicating a fragment is free to satisfy. Unlike probabilities, the cost domain already encodes impossibility separately as INFINITY/f64::MAX, so zero isn't a sentinel there; it's a real number PositiveF64 can't represent.

As always, as soon as I pull on one thing with this library it turns out there's a whole ediface of weird hacks that come tumbling down..

To be fair, the initial commit was 8 years ago.. and a lot has evolved overtime.

[apoelstra]

See #988

[apoelstra]

Costs cannot be PositiveF64s. Costs are routinely 0 (and as you say, in one case they are infinity). Only probabilities are guaranteed to be positive.

But even probabilities are set to 0 sometimes in the current compiler, notably when handling the andor construction.

[Abeeujah]

Superceded by #988