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

Why do Array / Hash not have .grab like QuantHashes do? #1658

Closed
lizmat opened this issue Mar 27, 2018 · 14 comments
Closed

Why do Array / Hash not have .grab like QuantHashes do? #1658

lizmat opened this issue Mar 27, 2018 · 14 comments
Assignees
Labels
consensus needed Needs a well-versed decision with justification, possibly from a core developer NYI Features not yet implemented tests needed Issue is generally resolved but tests were not written yet

Comments

@lizmat
Copy link
Contributor

lizmat commented Mar 27, 2018

Seems like an easy enough thing to implement for consistency / user expectance

@lizmat lizmat added NYI Features not yet implemented consensus needed Needs a well-versed decision with justification, possibly from a core developer labels Mar 27, 2018
@zoffixznet
Copy link
Contributor

zoffixznet commented Mar 27, 2018

Isn't .grab weighted? There are no weights on Array/Hash, so it makes sense there's no grab on them.

It's not supported on MixHash either, due to fractional weights:

<Zoffix__> m: say MixHash.new((a => 10000.1, b => 0.0001)).grab
<camelia> rakudo-moar a3da6ac56: OUTPUT: «.grab is not supported on a MixHash␤  in block <unit> at <tmp> line 1␤␤»

So there's no lack of consistency in available methods.

@thoughtstream
Copy link

There are no weights on Array/Hash

I would argue that weighting is merely a feature of .grab (as applied to QuantHash),
not the defining characteristic of .grab (as applied to all mutable containers).
At its heart, .grab is just a destructive .pick.

So QuantHash::grab has weighting because QuantHash::pick does;
whereas Array::grab and Hash::grab would not need weighting,
because Array::pick and Hash::pick don't.

As for MixHash, it doesn't offer .grab at all, but not because of fractional weighting.
Rather, MixHash doesn't offer .grab because it doesn't offer .pick.
Because neither operation is well-defined under fractional weighting.

This, to me, is actually an argument in favour of Array::grab and Hash::grab.
It would seem more consistent to me that any mutable that offers .pick
should offer .grab as well.

so it makes sense there's no grab on them

This issue arose when I was building a simple quicksort demonstration
and wanted to change from a first-element pivot:

my $pivot = @list.shift;

to a random pivot:

my $pivot = @list.grab;

which I expected to work...but which, to my chagrin, didn't.
Instead, I am currently forced to use the LTA:

my $pivot = @list.splice(@list.rand.Int, 1);

.grab (i.e. destructive .pick) just seemed the obvious tool here
and I was unhappily surprised when it didn't work.

@zoffixznet
Copy link
Contributor

Yeah, .grab as a destructive form of .pick makes sense.

@lizmat lizmat self-assigned this Mar 27, 2018
@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

There is actually a bit of an issue wrt Map and Hash here. .roll and .pick on Map return Pairs

dd {a => 42}.pick   # :a(42)

On QuantHashes , this is called .pickpairs.

So in that respect, we're looking at a compatibility / expectance issue.

@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

And then we have Supply.grab, which does something else entirely :-(

@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

Proof of concept of Array.grab implemented with a0e5e88

@zoffixznet zoffixznet added tests needed Issue is generally resolved but tests were not written yet and removed NYI Features not yet implemented labels Mar 28, 2018
@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

Well, it was intended as a proof-of-concept.

And there's still the matter of the semantics of Hash.grab in the light of Hash.pick semantics.

@lizmat lizmat added the NYI Features not yet implemented label Mar 28, 2018
@zoffixznet
Copy link
Contributor

Ah, right.

And there's still the matter of the semantics of Hash.grab in the light of Hash.pick semantics.

I think everything still makes sense if you squint enough and detach yourself from knowledge that BagHash is implemented as a Hash, which can be seen as an implementation detail:

  • .grab is a destructive .pick
    • Hash.pick returns a key/value Pair → Hash.grab returns a key/value Pair that it removed from the Hash
    • BagHash.pick returns one of the items in the bag → BagHash.grab returns an item from the BagHash that it removed

In other words, they're conceptually different: a Hash is a collection of key/value Pairs. A BagHash is a collection of things. Those things have weights, but the link between an item and its weight is a lot looser than between a key and value in a Hash. An BagHash can be implemented with an Array, there items are merely counted when their weight is required. While in a Hash the value is part of the "thing".

So in this view, a .grab both in a BagHash and in a Hash grabs one "thing". It just happens that in a Hash that thing is a Pair.


A counter-argument is that iterating over a BagHash iterates over Pairs, not individual items, and Arrays ain't got a .pickpairs that would pairify the picked items.

@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

Proof of concept implementation of (native array).grab with a393ff6

@lizmat
Copy link
Contributor Author

lizmat commented Mar 28, 2018

The confusing thing in this is that we also have BagHash.grabpairs, which is basically the functionality you propose for Hash.grab. And there I feel a mental impedance :-)

@thoughtstream
Copy link

Awesome work on Array::grab, Liz!

Hmmmm. It's probably too late to retcon it all now, but I'd much
rather just be able to think of .pick and .grab as special
types of accessor methods: as variants of the .[$n] and .{$k}
methods; variants that select at random, instead of by index/key.

Specifically, it would be much easier and more consistent/predictable
if .pick always returned whatever kind of result the equivalent
.[$index] or .{$key} (or .[@indices] or .{@keys}) returns
on the same datatype.

And then .grab would always return whatever kind of result
the equivalent .[$n] :delete or .{$k} :delete returns.

In which case, maybe the .pickpair and .grabpair variants are overkill
and .pick and .grab just need to be able to take a :p adverb instead:

$pivot-pair = @list.grab :p;                # formerly: .grabpair

@shuffled-pairs = %candidates.pick(*) :p;   # formerly: .pickpair

(in which case, perhaps they should also support :k, :v, and :kv as well?)

Hmmmmm.

@lizmat
Copy link
Contributor Author

lizmat commented Mar 29, 2018

To further confuse the matter: BagHash.grabpairs actually has a different semantic than BagHash.grab in that .grabpairs is unweighted. Which, if we're talking about a consistent interface, brings problems with BagHash.grab(:p): is that then weighted or not?

@lizmat
Copy link
Contributor Author

lizmat commented Mar 29, 2018

Also note that a List.grab with any adverb but :v would be useless, as the position is not valid anymore after having been grabbed. Although I guess you could argue that that would be the position to insert it again. But that would only always be true of no other grabbing had taken place since, contrary to a Pair from a Hash.

@thoughtstream
Copy link

Okay, so I completely misunderstood the purpose and semantics of .grabpairs.

Forget the adverb suggestion, which was not sensible, given what .grabpairs actually does, and which is not even very useful on .grab.

(Okay, maybe adverbs would still be useful on Hash::grab, but that should probably just return a pair, like Hash::pick does. After all, you can always chain a .key or .value if necessary.)

@lizmat lizmat closed this as completed Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus needed Needs a well-versed decision with justification, possibly from a core developer NYI Features not yet implemented tests needed Issue is generally resolved but tests were not written yet
Projects
None yet
Development

No branches or pull requests

3 participants