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

fix method overwrites during build #25212

Closed
wants to merge 2 commits into from
Closed

fix method overwrites during build #25212

wants to merge 2 commits into from

Conversation

JeffBezanson
Copy link
Member

Some recent changes added method overwrite warnings to the build, which is bad. This fixes them.

@fredrikekre
Copy link
Member

The idea with postfixapostrophize is that users can define methods for it, for their own types, and be able to use ' (rather than defining a method for Adjoint which might not make any sense). Perhaps @Sacha0 can shed some more light on this too

@JeffBezanson
Copy link
Member Author

Ok, I can work with that. How about calling it apostrophe instead?

@Sacha0
Copy link
Member

Sacha0 commented Dec 20, 2017

Thanks for fixing the overwrite Jeff! :)

As Fredrik mentioned, the aims of lowering to postfixapostrophize rather than Adjoint were to: (1) enable other uses of postfix ' without punning Adjoint; and (2) better separate the concerns of base and linalg.

Regarding spelling, I injected the qualification postfix for three reasons: (1) apostrophes see non-postfix use (e.g. for characters); (2) defining and/or calling this function should be rare, so length should not matter but explicitness and clarity do; and (3) if in the future apostrophes gain other uses, this name should remain clear. I selected the verb apostrophize rather than the noun apostrophe to reflect the function's semantics, i.e. application of a postfix apostrophe. Best!

@JeffBezanson
Copy link
Member Author

I don't really find postfixapostrophize that clear; it looks like letter salad to me. I also don't think ' will or should gain more uses, but we can cross that bridge when we get to it.

@Sacha0
Copy link
Member

Sacha0 commented Dec 20, 2017

I don't really find postfixapostrophize that clear; it looks like letter salad to me.

However you will then, I lack strong sentiments :).

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

Thanks Jeff!

@timholy
Copy link
Member

timholy commented Dec 21, 2017

For MethodTable, are you sure this is the right fix? If someone else wants that functionality, it could easily take an hour of poking around to discover how to reliably go from a method to the corresponding method table---given the implementation, it's hard to argue that the right steps are obvious. This approach was easily discoverable and did not add to the exports. Why not add a @skipwarning?

@timholy
Copy link
Member

timholy commented Dec 21, 2017

In #25222 I just moved it to methodshow.jl.

@JeffBezanson
Copy link
Member Author

There's been some discussion that ' should perhaps lower directly to adjoint, so this is still a bit up in the air.

@JeffBezanson
Copy link
Member Author

@Sacha0 and I decided that we should probably make adjoint and transpose lazy by default, and make (') === adjoint.

@JeffBezanson JeffBezanson deleted the jb/overwrites branch December 29, 2017 21:06
@fredrikekre
Copy link
Member

Did you also discuss what A' should result in without using LinAlg when LinAlg is in stdlib?

@JeffBezanson
Copy link
Member Author

No, but it would not be ideal for A' to change based on whether LinAlg has been loaded.

@fredrikekre
Copy link
Member

Agree, I think that A' should give an informative error about the need for using LinAlg or whatever. And I think that LinAlg should be the owner of adjoint, so we need something like apostrophe in Base that we can lower A' to, right? Then LinAlg would simply have Base.apostrophe(A) = adjoint(A).

@JeffBezanson
Copy link
Member Author

Or Base could contain an adjoint function with no methods.

@JeffBezanson
Copy link
Member Author

Any more thoughts on whether LinAlg could be more than one package? Maybe one with basic definitions like adjoint, one for BLAS, and then MatrixFactorizations? I think needing to load a package before A' works is not a good user experience, but we also don't want to have to load everything by default.

@Sacha0
Copy link
Member

Sacha0 commented Dec 31, 2017

Sharding LinAlg a bit sounds great. Seems like post-1.0 work though? Best!

@StefanKarpinski
Copy link
Member

I have to say, while I understand how we got here, needing to do import LinAlg in order to use A' makes me rather sad. If it wasn't syntax it would feel so unfortunate.

@Sacha0
Copy link
Member

Sacha0 commented Dec 31, 2017

Ah, I've been assuming that LinAlg will be loaded by default at the REPL, so that the import LinAlg isn't necessary?

@StefanKarpinski
Copy link
Member

This is a bit subtle. The code for stdlib is pre-loaded, but you would have to do using to get the names it exports. In this case, however, ' is already defined in Base. However, relying on it being pre-loaded means that we can't really decouple it later. Maybe that's fine and it will force us to separate the parts of LinAlg that provide functionality in Base versus parts of it that only exist to implement functionality that is exported from LinAlg.

@JeffBezanson
Copy link
Member Author

We do have the option of putting using LinAlg in the default juliarc. But I still think it makes sense to separate LinAlg into commonly-used stuff like dot products, vs. more obscure matrix factorizations.

@StefanKarpinski
Copy link
Member

I could see MatrixTypes and MatrixFactorizations stdlib packages.

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.

6 participants