Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

CI: Disable numpy avx512 instructions#22099

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 2 commits intomatplotlib:mainfromgreglucas:numpy-cpu-avx512
Jan 4, 2022

Conversation

greglucas
Copy link
Contributor

PR Summary

This disables the AVX512 instruction set at runtime within the CI using theNPY_DISABLE_CPU_FEATURES environment variable. This is due to small floating point differences causing test failures when using that instruction set on numpy 1.22 wheels on the GitHub Actions runners.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

This removes the NPY_DISABLE_CPU_FEATURES flag from the sphinx and tk testsas they emit warnings on CI which leads to failure from the subprocess.These don't need to be disabled on these tests, so remove them fromthe environment variables that are passed in.
@timhoffmtimhoffm added this to thev3.5.2 milestoneJan 3, 2022
@jklymak
Copy link
Member

This is pretty obscure. Does it make sense to just up the test tolerance instead? Though I think a couple of tests werequite wrong. Why did these flags have such a large effect?

@dstansby
Copy link
Member

This is pretty obscure. Does it make sense to just up the test tolerance instead? Though I think a couple of tests werequite wrong. Why did these flags have such a large effect?

I don't think any were unexpectedly wrong. I downloaded the failed images and all were minor chnages apart fromerrorbar_mixed, which was a large change because (I presume) a small difference was causing the axes limit to jump from 1e-2 to 1e-3.

I think this is the most pragmatic way to go forward, without having to play whack-a-mole with test tolerances across the test suite.

@jklymak
Copy link
Member

Oh I have the opposite point of view. If we have tests that are susceptible to compiler foibles maybe we should fix the tests or increase their tolerance, because there will always be more compiler foibles.

@greglucas
Copy link
ContributorAuthor

I agree this is a pretty big hammer, saying we won't test against certain floating point instruction sets. However, I assume we rely on numpy for some verification that their floating point calculations are "good enough" for them and this is impacting us because we are doing pixel-perfect comparisons with floating point inputs. I can see both arguments here, so I'm not sure which one people would be more generally comfortable with (increased tolerances, or reduced instruction sets).

There are only a few tests failing, so the tolerances on a few of them could probably be bumped, but the errorbar one we should probably update the autoscaling in that test by just bumping thenp.minimum() call to add in a small epsilon rather than adding a large tolerance.

@timhoffm
Copy link
Member

timhoffm commentedJan 3, 2022
edited
Loading

We have to live with the fact that our dependencies may introduce minor variations. First and foremost we don't want brittle tests. There are two ways to get there:

  1. Make the test environment as reproducible as possible
  2. Increase tolerances

I argue that if we can easily get away with (1) that's better than (2). We have more control and don't blindly accept other changes as well. For the same reason we pin freetype or remove text overall.

@timhoffm
Copy link
Member

I merge as is to unbreak the builds.@jklymak if you think tolerances are the better approach we can discuss this in the next call, and then maybe change.

jklymak reacted with thumbs up emoji

@jklymak
Copy link
Member

Sure, I've added to the call agenda.

timhoffm reacted with thumbs up emoji

dstansby added a commit that referenced this pull requestJan 4, 2022
…099-on-v3.5.xBackport PR#22099 on branch v3.5.x (CI: Disable numpy avx512 instructions)
@greglucasgreglucas deleted the numpy-cpu-avx512 branchJanuary 4, 2022 15:23
@tacaswell
Copy link
Member

I may have over-learned from our issue we had with "rendering the wrong glyph but tests are passing", but I am very very concerned about bumping tolerances in anything but a per-test basis.

@jklymak
Copy link
Member

Sure, I wasn't suggesting a general large tolerance. But here we have one test that had a large change because of floating point slop. So in my opinion that test should be made more robust. The other changes are pretty small, so upping those tolerances would also make sense to me.

@greglucas
Copy link
ContributorAuthor

Here is a link to a commit that would be what@jklymak suggests to increase tolerances/dtypes. All of them were pretty small.
greglucas@c030e05
I tried to move all of the failing tests to uselongdouble rather than increasing tolerances, but there ended up being some unsafe downcasting to float64 in the qhull procedures of trisurf.

@tacaswell
Copy link
Member

I came acrosshttps://etna.math.kent.edu/vol.52.2020/pp358-369.dir/pp358-369.pdf recently which has a case where increasing the precision of floats can make things worse (sometimes the rounding is actually in your favor!).

@tacaswell
Copy link
Member

@greglucas can you open a PR with those tolerances, they seem reasonable to me.

@greglucas
Copy link
ContributorAuthor

Sure, see#22132.

That is an interesting finding! I didn't think about trying single precision instead in the update...

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

@timhoffmtimhoffmtimhoffm approved these changes

@dstansbydstansbydstansby approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.2
Development

Successfully merging this pull request may close these issues.

5 participants
@greglucas@jklymak@dstansby@timhoffm@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp