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

View hint #303

Closed
jariji opened this issue Sep 20, 2022 · 10 comments · Fixed by #304
Closed

View hint #303

jariji opened this issue Sep 20, 2022 · 10 comments · Fixed by #304

Comments

@jariji
Copy link

jariji commented Sep 20, 2022

Currently subset takes view= which is a hint that may be ignored. I don't like the idea of ignoring keyword argument values, so I'm considering the issue. On Slack some approaches were discussed,

One option is to rename view to viewhint.

Another option is the view could be a hint supplemented by a second argument force or strict. I normally prefer that force/strict arguments be explicit about what exactly is being forced, for example forceviewoption.

But then once that's the situation, it seems odd to have both view and an option for saying "I really mean it" rather than just passing either viewhint or view with the latter being enforced. The enforced view argument could be added in a separate PR, which would make this option very similar to the first.

Therefore I propose renaming view to viewhint. Then in the future if users so desire, add a view argument that would be enforced. They would combine according to the following rules:

@match (view, viewhint) begin
(true, _) => enforce true
(false, _) => enforce false
(nothing, true) => prefer true
(nothing, false) => prefer false
end

An alternative future implementation could define a subsetview function if desired.

@bkamins
Copy link
Member

bkamins commented Sep 20, 2022

In relation to this proposal the reason I preferred view + force/strict is that these two kwargs are more orthogonal. In your proposal passing e.g. view=true, viewhint=false allows semi-contradition. Of course it is clear what happens following the rules you passed but typically I prefer arguments that do not have overlapping meaning if possible.
A small side benefit is that Tables 1.8.1 is already released and view has already a meaning so following your proposal would be technically breaking (fortunately probably no one relies on this functionality yet so maybe it could be acceptable as a patch).

@jariji
Copy link
Author

jariji commented Sep 20, 2022

The sense in which force is not orthogonal is that the meaning of one option affects the meaning of another, not just the overall outcome. Either way viewhint is a better name for a view hint than view is.

The main practical issue for me is that I don't want to be misled by a view=true that doesn't produce a view, or view=false that does.

@ToucheSir
Copy link

Base has the internal function maybeview (used by @views, among others) for the "prefer" case. Normalizing terminology here could be good.

@bkamins
Copy link
Member

bkamins commented Sep 21, 2022

Either way viewhint is a better name for a view hint than view is.

Yes, but Tables.jl 1.8.1 is already released.

@quinnj - are you OK to change the name of the kwarg from view to viewhint or maybeview in patch release? (it would be good to decide on it rather quickly if we are to make this change)

@bkamins
Copy link
Member

bkamins commented Sep 21, 2022

The conclusion from the discussion is to change the name from view to viewhint.

@quinnj - do you prefer to deprecate view or just remove it and treat the old name as a bug?

@quinnj
Copy link
Member

quinnj commented Sep 21, 2022

I think it's pretty easy to deprecate, so might as well do that.

@quinnj
Copy link
Member

quinnj commented Sep 21, 2022

PR up: #304

@ablaom
Copy link
Contributor

ablaom commented Sep 22, 2022

Base has the internal function[ maybeview ] (https://github.com/JuliaLang/julia/blob/2168230c210d620786311d91528a155582f78bc8/base/views.jl#L145)(used by @views, among others) for the "prefer" case. Normalizing terminology here could be good.

I vote for maybeview.

@bkamins
Copy link
Member

bkamins commented Sep 22, 2022

maybeview has a more narrow definition:

maybeview is like getindex, but returns a view for slicing operations
(while remaining equivalent to getindex for scalar indices and non-array types)

Maybe it is not relevant for many people, but e.g. we use maybeview in DataFrames.jl and it has this very precise meaning. In our case we are more flexible so I would prefer viewhint.

@bkamins bkamins linked a pull request Sep 22, 2022 that will close this issue
@ablaom
Copy link
Contributor

ablaom commented Sep 22, 2022

Got it. Thanks for the clarification!

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 a pull request may close this issue.

5 participants