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

Add docstring to promote_op #25689

Merged
merged 1 commit into from Jan 25, 2018
Merged

Add docstring to promote_op #25689

merged 1 commit into from Jan 25, 2018

Conversation

martinholters
Copy link
Member

Closes #25609.


!!! warning
In pathological cases, the type returned by `promote_op(f, argtypes...)` may not even
be a supertype of the return value of `f(::argtypes...)`. Therfore, `promote_op` should
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I assume this is due to _default_type? Unfortunate...

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Also typo in Therfore

@simonbyrne
Copy link
Contributor

I guess my main objection is that we do use it for preallocation of an output arrays (e.g. in matrix multiplication): is this unavoidable, or are there known safe cases?

Also, how does it differ from just calling Base._return_type directly?

@martinholters
Copy link
Member Author

I guess my main objection is that we do use it for preallocation of an output arrays (e.g. in matrix multiplication): is this unavoidable, or are there known safe cases?

I guess neither. It is just way simpler than an efficient implementation of the alternative (basing the type on the actual elements).

In addition to the empty case, I think I have found one more case where we have a lack of better alternatives: lazy views. What should the element type of an Adjoint be? Figuring that out by calling adjoint on all its elements would completely defeat its laziness.

Also, how does it differ from just calling Base._return_type directly?

julia> Base._return_type(*, Tuple{Signed, Signed})
Any

julia> Base.promote_op(*, Signed, Signed)
Signed

promote_op uses a heuristic to provide a more useful result here. The problem is that if someone adds a method to * for some custom Signed type that does not return a Signed, promote_op still gives the same answer.

@davidanthoff
Copy link
Contributor

In addition to the empty case, I think I have found one more case where we have a lack of better alternatives: lazy views.

Just to note that I have exactly the same issue in Query.jl. You can think of the design there as a series of chained, lazy iterators, and the only way to know what the element type of the last one in that chain is, is to rely on inference. I'm doing away with this right now, but the downside is that IteratorEltype on that last iterator will have to return EltypeUnknown, which is a bit of a shame. I think that is just how it is, just wanted to let you know about another use-case :)

@JeffBezanson
Copy link
Sponsor Member

This function really is hard to justify. return_type is unpredictable, but at least it is sound. promote_op manages to be both unpredictable and unsound. First, it substitutes different types than the ones you're actually asking about. Then it calls return_type, and then tries to paper over its issues with some typejoins. Those typejoins still don't ensure you'll get a correct answer, and yet they also make the result less precise than it could be.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Jan 24, 2018

promote_op is a travesty. It really should not exist. If it didn't do the type substitution thing and was actually sound it would be far more justifiable. Then it would just be a wrapper around type inference to be used in corner cases where we have no actual elements to derive types from.

@martinholters
Copy link
Member Author

While I certainly share the aversion against promote_op, I have to admit it works surprisingly well in practice. Which makes it all the more dangerous by being more seductive.

Anyway, I've fixed the typo Jeff pointed out, there were no other objections, so here we go.

@martinholters martinholters merged commit a101ef4 into master Jan 25, 2018
@martinholters martinholters deleted the mh/promote_op_doc branch January 25, 2018 10:28
@martinholters
Copy link
Member Author

martinholters commented Jan 25, 2018

The Masters office. Behind his desk, The Master is deeply concentrated, working in front of his computer.
A knock on door.
Master: Yes!
A young programming apprentice enters.
Apprentice: Excuse me, Master. I was just implementing a function which needs to allocate an array for its return values, and wanted to use promote_op to decide the element type. But now I've learned that might be a bad idea. Say, Master, what should I do now?
Master: I see you do read the docs. That is good. You will progress quickly. I have high hopes with you. Now to your problem: Fear not! Just use return_type directly and all will be good.
Apprentice: Thank you, Master, for sharing your wisdom.
The apprentice leaves. The Master returns to his work.
Briefly thereafter, it knocks again.
Master: Yes!
The young programming apprentice enters.
Apprentice: Excuse me, Master, I've followed your advice and use return_type, but often, the type will be too wide. Sometimes I even get Any. How can I get a narrower type?
Master: Ah, that my young apprentice, is simple: Just sprinkle enough @pures in your code.
Apprentice: Thank you, Master, for sharing your wisdom.
The apprentice leaves.
The Master rips off his mask and laughs a hollow, frightening laugh.

@StefanKarpinski
Copy link
Sponsor Member

This should be the promote_op docstring. Not kidding – at least part of it.

@simonbyrne
Copy link
Contributor

That tops magicfunction.

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

5 participants