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

LinearAlgebra remove unused export #28684

Merged
merged 1 commit into from
Aug 16, 2018
Merged

LinearAlgebra remove unused export #28684

merged 1 commit into from
Aug 16, 2018

Conversation

KristofferC
Copy link
Sponsor Member

Maybe we should CI that nothing exports undefined symbols?

@fredrikekre fredrikekre merged commit e23515d into master Aug 16, 2018
@fredrikekre fredrikekre deleted the KristofferC-patch-4 branch August 16, 2018 11:17
@Jutho
Copy link
Contributor

Jutho commented Aug 16, 2018

Is it actually useful to export all these factorization types. I don't think they are often constructed explicitly/directly in user code? I sometimes have a name clash while scripting, because I have a variable that is called QR (which has nothing to do with the QR).

KristofferC added a commit that referenced this pull request Aug 19, 2018
KristofferC added a commit that referenced this pull request Aug 19, 2018
@KristofferC KristofferC mentioned this pull request Aug 19, 2018
KristofferC added a commit that referenced this pull request Aug 19, 2018
@Jutho
Copy link
Contributor

Jutho commented Aug 21, 2018

Bump. I assume this is not the right place to ask, but the questions seems to small to open a new issue or Discourse thread. Is it useful that all these types are exported? More generally, should the export list of the standard libraries be that what used to be exported by Julia Base, or the whole list that used to be exported in the corresponding submodules of Base back in the 0.6 days?

@fredrikekre
Copy link
Member

Is it useful that all these types are exported?

Which ones are not useful?

More generally, should the export ...

Yea, I think it is ok for LinearAlgebra to export more stuff than what was reexported from Base, since it is required to using LinearAlgebra to make them visible in your module.

@Jutho
Copy link
Contributor

Jutho commented Aug 21, 2018

Which ones are not useful?

My apologies if I was not clear; I meant the factorization types, i.e.

    Factorization,
    BunchKaufman,
    Cholesky,
    CholeskyPivoted,
    Eigen,
    GeneralizedEigen,
    GeneralizedSVD,
    GeneralizedSchur,
    Hessenberg,
    LU,
    LDLt,
    QR,
    QRPivoted,
    LQ,
    Schur,
    SVD,

I don't think these are ever directly called or constructed, but only arise as output of the corresponding lowercase functions. This is different from e.g. Symmetric or UppperTriangular which are directly called for construction, and should indeed definitely be exported.

@andreasnoack
Copy link
Member

I think it would be fine not to export the factorization types. As you point out, normal users should never need to use them explicitly.

KristofferC added a commit that referenced this pull request Sep 8, 2018
KristofferC added a commit that referenced this pull request Sep 8, 2018
@KristofferC KristofferC added kind:bugfix This change fixes an existing bug and removed backport pending 1.0 labels Sep 27, 2018
KristofferC added a commit that referenced this pull request Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants