Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update and expand CI jobs #215

Merged
merged 9 commits into from
May 27, 2024
Merged

Conversation

FreezyLemon
Copy link
Contributor

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
Copy link
Contributor

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?

@FreezyLemon FreezyLemon force-pushed the update-ci branch 3 times, most recently from e931982 to a6e9c39 Compare April 29, 2024 12:03
- Disable incremental compilation (useless on CI)
- Run clippy first before test jobs
- Add `-Dwarnings` to fail on compiler warnings
- Run on macos, linux and windows
- Remove explicit build target (host arch is fine)
@YeungOnion
Copy link
Contributor

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
Copy link
Contributor Author

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.

This case caused a test failure on macOS
Copy link
Contributor

@YeungOnion YeungOnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprised it's only one, that's nice. Thanks!

@YeungOnion YeungOnion merged commit 2e3c453 into statrs-dev:master May 27, 2024
8 checks passed
@FreezyLemon FreezyLemon deleted the update-ci branch May 27, 2024 07:24
@YeungOnion YeungOnion mentioned this pull request Jun 7, 2024
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.

2 participants