Make almost_eq return false if a and/or b is NaN

[FreezyLemon]

I feel like this might have been a typo because the docs say that almost_eq always returns false if either number is NaN. It also seems correct that almost_eq(NaN, x) is always false.

[YeungOnion]

This is certainly more correct. However, I notice that there's some inconsistency with using prec::almost_eq and things coming from approx:: despite a type constraint to impl ::approx::RelativeEq<f64>

Would you be willing to write this method as simply a wrapper to appropriate call in approx? Perhaps,

a.abs_diff_eq(b, other)

A bigger ask would be to drop the requirement on approx which seems feasible.

[FreezyLemon]

I implemented the abs_diff_eq wrapper for now. EDIT: Please take a look at the test failures, this seems to have introduced a change in behavior..

A bigger ask would be to drop the requirement on approx which seems feasible.

I agree, it seems feasible. Is there a particular reason for dropping the approx dependency though? It seems to be a widely-used crate and is pretty lightweight.

[YeungOnion]

I see two areas needing fix, but the first would solve both issues

1. no handling of infinity, works if handled in prec::almost_eq
2. verifying that d.ln_pdf(x).abs_diff_approx_eq(d.pdf(x).ln(), prec) in internal::test_continuous which should be verifying that the relationship between cdf and pdf via integration from min domain to max domain.

I pushed the first here to fix, but perhaps it makes more sense to test ln_pdf accuracy elsewhere

EDIT: forgot to say where handling INF would work in in line item 1

[FreezyLemon]

Doesn't this code now return true for almost_eq(f64::INFINITY, f64::NEG_INFINITY, /* anything */)?

[YeungOnion]

🤦🏼, thanks for catching.

I think I'm just getting antsy for the release and quickly merged the changes I pushed. I shouldn't be skipping the chance for peer review.

On a related note, do you mind if I push (no force) on your fork's branches that have open PRs? I didn't think to ask before, but it seems like a courtesy to request that permission.

[YeungOnion]

perhaps

a == b || a.abs_diff_eq(&b, acc)

would be more appropriate.

[FreezyLemon]

I think I'm just getting antsy for the release and quickly merged the changes I pushed. I shouldn't be skipping the chance for peer review.

It's a small change which doesn't usually warrant a full review. It's just a mistake, and mistakes happen. No big deal.

On a related note, do you mind if I push (no force) on your fork's branches that have open PRs? I didn't think to ask before, but it seems like a courtesy to request that permission.

Thank you for asking, but I don't mind at all. I would've disabled it via GitHub if it did bother me.

a == b || a.abs_diff_eq(&b, acc)

Sure, that should work. Might read better as

if a.is_infinite() && b.is_infinite() {
  return a == b;
}

though.

[YeungOnion]

It's just a mistake, and mistakes happen. No big deal.

Appreciate you saying that.

Might read better as ...

Ah, the intent is more explicit.