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

clarify documentation for @view #39542

Merged
merged 3 commits into from
Feb 10, 2021
Merged

clarify documentation for @view #39542

merged 3 commits into from
Feb 10, 2021

Conversation

simeonschaub
Copy link
Member

There was some confusion about this on Slack for the case of broadcasted assignment.

@simeonschaub simeonschaub added docs This change adds or pertains to documentation arrays [a, r, r, a, y, s] labels Feb 5, 2021
base/views.jl Outdated Show resolved Hide resolved
Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

I don't really like calling out broadcast here as it's a no-op. Broadcast already does it for you.

base/views.jl Outdated Show resolved Hide resolved
base/views.jl Outdated Show resolved Hide resolved
@mbauman
Copy link
Member

mbauman commented Feb 5, 2021

Oh, I missed the emphasis on updating broadcasts. Yes, that usage makes sense... but maybe we should just do it by default anyways?

@simeonschaub
Copy link
Member Author

I don't really like calling out broadcast here as it's a no-op. Broadcast already does it for you.

Not for the @view(A[1, 2:end]) .*= 2 example though, because it's allocating otherwise. I do really like all your other suggestions, I think it explains the purpose of this macro much better.

@simeonschaub
Copy link
Member Author

but maybe we should just do it by default anyways?

Yeah, I thought about this before as well. It would violate the assumption that A[idx] .*= 2 is just equivalent to A[idx] .= A[idx] .* 2, although that's already not true if you have any function with side effects on the lhs.

@simeonschaub
Copy link
Member Author

@mbauman Do you still think we should not mention the @view(A[1, 2:end]) .*= 2 example here? If so, I'd just go ahead with your suggestions, but personally, I think it wouldn't hurt to mention that example here as well, since I find it a bit non-obvious. I don't feel to strongly about this though.

@mbauman
Copy link
Member

mbauman commented Feb 9, 2021

Yeah, I think it's fine and even helpful to mention — my review was thinking this was calling out the difference between = and .=. I've edited my suggestion above. It's pretty wordy, but it's an attempt at making this a bit clearer.

@simeonschaub simeonschaub merged commit ce5bd1c into master Feb 10, 2021
@simeonschaub simeonschaub deleted the sds/view_doc branch February 10, 2021 11:54
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* clarify documentation for at-view

* fix typo

* Apply suggestions from code review

Co-authored-by: Matt Bauman <[email protected]>

Co-authored-by: Matt Bauman <[email protected]>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* clarify documentation for at-view

* fix typo

* Apply suggestions from code review

Co-authored-by: Matt Bauman <[email protected]>

Co-authored-by: Matt Bauman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants