Skip to content

Use lapacke library, drop linking to LAPACKE implementation #95

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 20, 2017
Merged

Use lapacke library, drop linking to LAPACKE implementation #95

merged 2 commits into from
Dec 20, 2017

Conversation

alexbool
Copy link
Contributor

@alexbool alexbool commented Dec 3, 2017 •
edited
Loading

Adapt for changes in the upstream crate (was lapack, now C wrappers are in lapacke) and do not link to an LAPACKE implementation in this library. Linking to an LAPACKE implementation now starts to be user's responsibility and allows for more flexibility than with Cargo features, for example, selecting different backend for different OS.

Relevant lapack commit: blas-lapack-rs/lapack@5516490
Also includes minor changes from #94.

typot[bot]
typot bot reviewed Dec 3, 2017
View reviewed changes
Copy link

@typot typot bot left a comment

Choose a reason for hiding this comment

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

Review from typot

README.md Outdated
@@ -10,23 +10,38 @@ Dependencies
-------------

- [bluss/rust-ndarray](https://github.com/bluss/rust-ndarray)
- [stainless-steel/lapack](https://github.com/stainless-steel/lapack)
- [stainless-steel/lapacke](https://github.com/stainless-steel/lapacke)
Copy link

Choose a reason for hiding this comment

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

"stainless-steel/lapacke" at 13 is typo?

README.md Outdated
@@ -10,23 +10,38 @@ Dependencies
-------------

- [bluss/rust-ndarray](https://github.com/bluss/rust-ndarray)
- [stainless-steel/lapack](https://github.com/stainless-steel/lapack)
- [stainless-steel/lapacke](https://github.com/stainless-steel/lapacke)
Copy link

Choose a reason for hiding this comment

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

"-" at 13 is typo?

  • e
  • s
  • i
  • a
  • n

README.md

and more (See Cargo.toml).

Feature flags
--------------
Choosing LAPACKE implementation
Copy link

Choose a reason for hiding this comment

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

"LAPACKE" at 17 is typo?

  • LAPLACE
  • LACKEY
  • LACKER
  • PLACKET
  • FLAPJACK

README.md
Feature flags
--------------
Choosing LAPACKE implementation
--------------------------------
Copy link

Choose a reason for hiding this comment

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

"--------------------------------" at 18 is typo?

README.md
Choosing LAPACKE implementation
--------------------------------

For the sake of linking flexibility, you must provide LAPACKE implementation (as an `extern crate`) yourself.
Copy link

Choose a reason for hiding this comment

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

"LAPACKE" at 20 is typo?

  • LAPLACE
  • LACKEY
  • LACKER
  • PLACKET
  • FLAPJACK

README.md
`main.rs`:
```rust
extern crate ndarray;
extern crate ndarray_linalg;
Copy link

Choose a reason for hiding this comment

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

"ndarray_linalg;" at 42 is typo?

  • supernaturalness
  • unnaturalness

README.md
```rust
extern crate ndarray;
extern crate ndarray_linalg;
extern crate openblas_src; // or another backend of your choice
Copy link

Choose a reason for hiding this comment

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

"openblas_src;" at 43 is typo?

  • impregnableness
  • exceptionable

README.md
```rust
extern crate ndarray;
extern crate ndarray_linalg;
extern crate openblas_src; // or another backend of your choice
Copy link

Choose a reason for hiding this comment

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

"//" at 43 is typo?

README.md
```rust
extern crate ndarray;
extern crate ndarray_linalg;
extern crate openblas_src; // or another backend of your choice
Copy link

Choose a reason for hiding this comment

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

"backend" at 43 is typo?

  • backed
  • back end
  • back-end
  • backhand
  • bracken

README.md
extern crate ndarray;
extern crate ndarray_linalg;
extern crate openblas_src; // or another backend of your choice
```
Copy link

Choose a reason for hiding this comment

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

"```" at 44 is typo?

@alexbool
Copy link
Contributor Author

alexbool commented Dec 3, 2017

CI failed, investigating

@alexbool alexbool force-pushed the update-lapack branch 2 times, most recently from 7006201 to 83715b9 Compare December 3, 2017 16:07
@alexbool
Copy link
Contributor Author

alexbool commented Dec 3, 2017

CI green, ready to merge I think.
Also please add wercker/unit-tests pipeline to CI (I had to extract it).

@termoshtt
Copy link
Member

Thanks PR, and sorry for later replay at #94

Your lapacke-provider is equal to lapack-src except that yours contains intel-mkl-src option instead of accelarate-src. I think we should use lapack-src after send a PR to add intel-mkl-src. It is discussed at following issues

rust-math/intel-mkl-src#10
blas-lapack-rs/lapack-sys#8

I intend to keep feature flags for easy testing like ndarray/Cargo.toml with no-link default feature. Then, users of this crate has two way to use backend:

ndarray-linalg = "*"
openblas-src = "*"

ndarray-linalg = { version = "*", features = ["openblas"] }

This will be both fully-customizable and easy-usable. In this way, we do not need to create separate testing crate like you created integration-tests since we can set the features by command line as done in current master. Your unit-tests should be replaced by cargo check which tests rust codes without linking.

BTW, do you know how to hide typot comment? It's too long page (´・ω・｀)

@alexbool
Migrate to lapacke library
2dcb205 
Also bumped version to 0.8
@alexbool
Copy link
Contributor Author

alexbool commented Dec 19, 2017 •
edited
Loading

OK, I've updated this PR according to your considerations.
As for now, it is not ready for merge because I use my own personal fork of lapack-src with intel-mkl feature.
Let's see what CI will say.

@termoshtt termoshtt merged commit 2dcb205 into rust-ndarray:master Dec 20, 2017
@termoshtt
Copy link
Member

Thanks your contribute. I merged with several fix.

@alexbool alexbool deleted the update-lapack branch January 29, 2018 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@typot typot[bot] typot[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexbool @termoshtt