robpike / ivy

ivy, an APL-like calculator
Other
1.32k stars 103 forks source link

Add `unique` operator #129

Closed Blackmane closed 1 year ago

Blackmane commented 1 year ago

Hello, I tried adding the unique operator, based on how the same operator works on APL https://aplwiki.com/wiki/Unique

The unique operator remove duplicates from a vector. It's implemented as a unary operator.

Some example of the implementation:

unique iota 9
1 2 3 4 5 6 7 8 9

unique 1
1

unique 1 1 1 1 1 1 1 1 1
1

unique 'Missisipi'
Misp

I hope it could be a useful operator. It's my first time with Go. Let me know if something in the implementation is missing.

robpike commented 1 year ago

Thanks for this, it's an interesting addition. Some important components are missing though. It should be implemented for matrix type. I believe in APL it just compares the top items (rows of a matrix for a 2d one, for instance) but that should be verified. Also, of course, you need tests.

I'm happy to take this over, or leave it with you. Let me know.

Blackmane commented 1 year ago

I can try to implement for the matrix type too. With a colleague I was discussing the implementation. Hopefully later we update the PR.

I also check for the missing components too. Thank you!

Blackmane commented 1 year ago

I have implemented unique for matrix too. I added some tests and a line of documentation. I'm not sure if is all or if something is still missing.

I'm open to any suggestion or modification.

robpike commented 1 year ago

Thanks for the update.

Your solution to the matrix operation is quite expensive, both in CPU time and in allocations. I believe that because of the way memory is laid out in matrices, which is not accidental, this can be done more efficiently. Also, there are some secondary operations that might be worth adding, either internally or explicitly in what is available to the user. There should probably be a equality operator for matrix and vector comparisons, and perhaps also a hash function.

Let me think about these for a while.

robpike commented 1 year ago

Here's my thinking so far.

It bothers me that your vector and matrix implementations are so different. They should be able to share a lot of their operation. But some pieces are missing to make that possible, such as general equality and general ordering. They could be provided, maybe.

Also, the "unique" operation in APL is declared as a monadic version of a general set operation. (I forget which one, but it doesn't matter for the moment.) Ivy should also have set operations, but to provide union and intersection would require similar mechanisms to what unique needs.

The key to all this is, I think, efficient search within a slice of Values. That is what both Vector and Matrix are under the covers.

I am thinking about the best way to provide that.

robpike commented 1 year ago

I've thought about this quite a bit, and unfortunately it's harder than it looks. Most relevant here, you can't put a Value in a map and be ivy-correct about the results because the (in effect) == operator used by the Go map implementation has different semantics than ivy's. For instance: (float 1) == 1 in ivy, but the comparison will yield false in the interface comparison used in map lookup. And that's leaving aside the panic that will occur should a Vector show up in the map.

This is turning into a bit of a rabbit hole but I have a handle on things now, and plan to add union, intersection, unique, and perhaps whole-object comparison operations (not element-wise). I have some of it working, if slowly, and once it's passing all the tests I'll see what I can do about making it fast, which may require some structural changes. And doing it with minimal code duplication is another challenge. Plus there are issues around char vs. int.

It's all fun, but this PR is not the place to start, and I'm going to close it.

That said, I'd like to thank you for raising the issue and taking it on, which should lead to some cool new functionality in a few days.

robpike commented 1 year ago

Done!

Fixed by 3d1d0ff6912e8c12725cec1c178f908c53f12d44

Blackmane commented 1 year ago

Amazing! Thank you!!