Skip to content

Add trivial smoketest for xpotrf #1318

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

Merged
merged 1 commit into from
Sep 30, 2017
Merged

Conversation

pv
Copy link
Contributor

@pv pv commented Sep 30, 2017

Add a trivial test for the *potrf routines to utest.

We've now been burned a few times by "bad" openblas builds (on some platforms only) that had broken cholesky factorizations. It would be useful to catch those already when building openblas itself, so here is a PR that adds a trivial smoketest that should catch those.

@matthew-brett
Copy link
Contributor

Guys - would you mind testing this patch on Windows 32-bit? That is where we are seeing the numpy failure.

@pv
Copy link
Contributor Author

pv commented Sep 30, 2017

The problematic case does not require just windows 32-bit, but also the specific compiler and maybe compiler flag choice, which I'm not sure is useful for openblas devs to spend time on. I tested myself that this test case correctly fails when using the "bad" build binaries, and it passes for correctly working potrf routines (as indicated by appveyor&travis).

@matthew-brett
Copy link
Contributor

At the moment, though, the only situation we know this fails, is on Windows 32-bit, with the given compiler / flags and so on; so if this test passes with a standard mingw build, on Windows 32-bit, then we (numpyers) can try and track down which of our tweaks is causing the problem.

@pv
Copy link
Contributor Author

pv commented Sep 30, 2017

@matthew-brett: the "standard" mingw-w64/32bit compiler shipped on appveyor produces problem-free binaries (indeed, remember that we already had this discussion vs. scipy). If necessary, let's discuss that elsewhere since it's probably not so relevant for openblas development.

Anyway, even if the failing compiler setup is rare, it would still be useful if the standard openblas test suite would have at least minimal testing of the potrf routines.

@matthew-brett
Copy link
Contributor

Ah - no - sorry - I had not registered that #1270 was the same issue.

@martin-frbg martin-frbg merged commit ebe8421 into OpenMathLib:develop Sep 30, 2017
@martin-frbg
Copy link
Collaborator

I'm all for detecting errors early, thanks :-)

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.

3 participants