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

lapack: move lapack/native => lapack #26

Closed
wants to merge 1 commit into from
Closed

Conversation

kortschak
Copy link
Member

This is the first half of the lapck/blas move. The second part has code gen fiddling that it's too late in the day to do.

The godoc gets pretty unwieldy so do we want to do this?

@btracey Please take a look.

@btracey
Copy link
Member

btracey commented May 26, 2017

The lapack/native godoc is unwieldly on its own. The question becomes how valuable it is to have the lapack types themselves be very visible (EVJob, etc.). I would guess one encounters these types either by looking at the lapack64 call to a function, or by knowing a specific lapack function to call. Either way, godoc.org will contain links to the specific type documentation. Seems like it'll be not so much worse to find type information (and specific function information will always be difficult to find).

The other suggestion is that maybe lapack.Float64 could be moved into lapack64. Unlike the BLAS interfaces, the Lapack interface is kind of our definition, and only really meaningful to lapack64. That shift in location would give visibility to all of the high-level functions we provide.

@kortschak
Copy link
Member Author

kortschak commented May 26, 2017

The lapack/native godoc is unwieldly on its own.

This is true, but I was considering more the increase in unwieldiness of lapack rather than of lapack/native. Even in the netlib packages we manage to hide the horror of the complete interface from the user by separating it out into lapacke.

The idea of moving lapack.Float64 to lapack64 seems reasonable, but it leaves us with a sort of inverted file system structure where the first directory encountered in the import paths is the implementation (which is logically secondary) and then the preferred user interface. There is no syntactic or language-semantic problem with this, but it does fly against the usual understanding and convention.

I think I'm falling on the side that the native packages should not be moved, though maybe renamed. This has the problem that in both case we end up with imports like "gonum.org/v1/gonum/pkg/pkg". Maybe this is the least worst.

Another alternative is to rename all so that we end up with e.g. "gonum.org/v1/gonum/lapack/native" and "gonum.org/v1/netlib/lapack/netlib" which then give us uses like lapack64.Use(native.Implementation{}) (or conceivably "gonum.org/v1/netlib/lapack/gonum" and lapack64.Use(gonum.Implementation{})) and lapack64.Use(netlib.Implementation{}). This mirrors the original reasoning behind the use of "cgo" and "native". Name collisions are usually unlikely and import renaming is possible if needed.

@btracey
Copy link
Member

btracey commented May 29, 2017

Sorry for the delay. I've been busy with visitors and also trying to think of the best course of action. The netlib/gonum.Implementation{} is appealing, but this causes the name collision that is likely. It's reasonably common to want to import gonum/netlib/blas/netlib and gonum/netlib/lapack/netlib.

This issue is tricky.

It seems like there are two fundamental problems. The first is circular imports, because we can't define the types in the same place as the wrapper function. The second is name clashes between Blas and Lapack.

We could solve the first problem with aliases. Define all of the types in native (or some renaming) , which are then aliased is the base lapack (with the wrapper functions). This could then be imported directly by netlib, and that's that. Not sure it's a good idea, but it is a resolution.

I thought we could maybe combine BLAS and Lapack into one package, but I don't think this can work. Lapack uses the BLAS implementation, which I think is an important property, and I don't see how we can usefully maintain that property while combining the packages.

Still, it seems like a place to start is to move the interface definitions and high-level wrapper functions into gonum/lapack and gonum/blas. We then need a sub-package for the base types (blas.Side) and a place for the implementation.

@kortschak
Copy link
Member Author

I don't want to use aliases here. I don't think they are necessary.

Still, it seems like a place to start is to move the interface definitions and high-level wrapper functions into gonum/lapack and gonum/blas. We then need a sub-package for the base types (blas.Side) and a place for the implementation.

I don't understand. Can you spell this out to me - I don't think splitting things up further is likely to be helpful.

To be honest, I like the structure as it exists, it's just the naming that causes concern.

As far as name clashes go with would see the following in use if both blas and lapack were imported in the same file.

import (
    blas   "gonum.org/v1/gonum/blas/gonum"
    lapack "gonum.org/v1/gonum/lapack/gonum"
)

func foo() {
    blas64.Use(blas.Implementation{})
    lapack64.Use(lapack.Implementation{})
    ... do stuff
}

I don't see that this is a problem, in fact it makes swapping out implementations marginally easier.

@btracey
Copy link
Member

btracey commented May 30, 2017

Yea, that suggestion is basically the same as now, and is fine.

I was pondering the consequences of the access point being lapack as you suggest. It seemed like one consequence is lapack64 should be combined with the hypothetical clapack128, which is actually nice in that it mirrors the future mat. The rest of the naming is hard though, and so maybe the current situation is best.

@kortschak
Copy link
Member Author

Abandoning.

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.

None yet

2 participants