search

statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
544 stars 78 forks source link

Update and expand CI jobs #215

Closed FreezyLemon closed 1 month ago

FreezyLemon commented 2 months ago

Partially addresses #207. This does not auto-fetch the NIST dataset, I'll explain why below.

This PR:

  1. Runs clippy in CI, failing on any warnings
  2. If clippy succeeds: Runs stable + nightly tests on macos, windows, linux

Example run on my fork (there are quite a few clippy warnings and errors on master, I ""fixed"" these to show the CI run). Looks like one of the nightly tests fails on macOS, maybe because of the ARM architecture.

Often, maintainers don't like merging commits which result in failing CI. So I don't mind having this PR wait until the clippy errors etc. are addressed.

As to why the NIST dataset is not handled (yet): The simple idea was to create a script that downloads the dataset before testing. However, the data on the NIST website is not in the same format as it is in this repo. The text files in this repo are raw line-by-line float values. The actual dataset I found online (link to one of them) links to an ASCII data file that includes a file header and is formatted differently. This would not work without at least a little bit of scripting work, which seems out of scope for this PR.

YeungOnion commented 2 months ago

Think it's fair to separate preprocessing the datafiles since it'll probably not be a shell one-liner.

It'll also be good to run cargo +nightly fmt, would you be willing to add that?

YeungOnion commented 1 month ago

Manually re-ran test on (ubuntu-latest, nightly) and it passes, not sure where the differences are occurring but it does drive the usage for not using assert_eq in tests. The test that fails first is accurate for the first 17 digits, i.e. better than good for f64.

I think it'll take a bit to do an honest review of all tests that use assert_eq in lieu of checking within specified precision, but it might be worth if/when users pick the new release up (and hopefully test again), thoughts?

FreezyLemon commented 1 month ago

Manually re-ran test on (ubuntu-latest, nightly) and it passes, not sure where the differences are occurring

macos-latest runs on M1 (arm64) hardware, ubuntu-latest on x86_64. That could explain the difference.

it does drive the usage for not using assert_eq in tests.

I agree. There might be exceptions in some circumstances though (x == 0 or x == 1).

I think it'll take a bit to do an honest review of all tests that use assert_eq in lieu of checking within specified precision, but it might be worth if/when users pick the new release up (and hopefully test again), thoughts?

It would most likely take a fair bit of time, yeah. I'm not sure it's very important to get that done quickly though, for now it should be enough to fix the real errors.

Regarding the actual error: The inequality happens at the 17th decimal place. IEEE doubles only really guarantee 15, often 16 decimal places of precision, so this behavior should be perfectly acceptable within IEEE spec. I'll push a fix for it.