Skip to content

Add all, any and find methods on vectors #15

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

Open
zesterer opened this issue Oct 26, 2018 · 9 comments
Open

Add all, any and find methods on vectors #15

zesterer opened this issue Oct 26, 2018 · 9 comments

Comments

@zesterer
Copy link
Contributor

zesterer commented Oct 26, 2018

Now that vek has methods such as map2 and apply2, methods such as all, any and find would have many potential uses. For example:

let v1 = Vec3::new(1, 2, 3);
let v2 = Vec3::new(3, 2, 1);

// Returns true if all of the elements satisfy the 'e > 5' predicate
let x = v1.map2(v2, |a, b| a + b * 3).all(|e| e > 5); // true

// Returns true if any of the elements satisfy the 'e == 8' predicate
let y = v1.map2(v2, |a, b| a + b * 3).any(|e| e == 8); // true

// Returns the index of the first element that satisfies the 'e == 8' predicate
let z = v1.map2(v2, |a, b| a + b * 3).find(|e| e == 8); // 1

In addition, it might be useful to add a few utility methods specifically for boolean vectors: .and() and .or(). These would provide a nice shortcut to the above code for Vec3<bool>-like types.

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 26, 2018

I've thought about this too; they are basically nice shortcuts for :

let v1 = Vec3::new(1, 2, 3);
let v2 = Vec3::new(3, 2, 1);

// Returns true if all of the elements satisfy the 'e > 5' predicate
let x = v1.map2(v2, |a, b| a + b * 3).map(|e| e > 5).reduce_and(); // true

// Returns true if any of the elements satisfy the 'e == 8' predicate
let y = v1.map2(v2, |a, b| a + b * 3).map(|e| e == 8).reduce_or(); // true

// Returns the index of the first element that satisfies the 'e == 8' predicate
let z = v1.map2(v2, |a, b| a + b * 3).iter().position(|e| e == 8).unwrap(); // 1

I'm a bit bothered by the fact that the primitive slice type doesn't have find() (it has contain(), so why not?); If it had, vector types would have it out-of-the-box simply because they Deref to slices.

In fact, all of the methods you describe deserve to be built-in methods of slice; they're quite fundamental and not specific to vek. C++ has std::all_of, std::any_of and std::none_of for this, actually. GLSL also has any, all and not.

These are convenient and easy to implement, so I'm all for adding them, but it's regrettable that they're not already part of the standard slice API.
I'll start work on it this evening; in the meantime if you have more ideas of other fundamental functions, feel free to suggest them. :)

@zesterer
Copy link
Contributor Author

Thanks! You are right, reduce_and and reduce_or make a lot more intuitive sense.

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 26, 2018

In fact most of C++'s STL algorithm header has nice-looking operations that can operate on most collections, including the special case of slices of numbers (which VecX types are); There's exclusive_scan and inclusive_scan which are somewhat interesting, for instance.

To me the only question is "how far do we go?" and there's no end-all answer; we have to be opinionated and there are trade-offs. any, all and none_of are simple, easy to implement, and easier to read that map + reduce_and/or, so I'm fine with adding them.

Also, in a vector math library I like to follow the rule "If GLSL has it, then it probably belongs in my library as well", which is the case here!

@zesterer
Copy link
Contributor Author

For me, as a user rather than a maintainer, I often hit situations in which I tell myself "damn, this is a wordy piece of code". For me, that's the point at which implementing these methods becomes useful.

Another big aspect is of course combinatorial functions. There's no point having a .is_divisible_5_but_greater_than_17() method because the same functionality could obviously be achieved through combinations of existing functions. So the priority should clearly be on a (relatively) small group of functions that are versatile enough to be combined in such a way as to produce almost any higher-order function one may require.

(Does vek have .fold()? That might be very useful)

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 26, 2018

.fold() is currently possible with .iter().fold(). Not sure if it deserves to be added, but after all there's already reduce() which shouldn't be there, but should have been implemented on iterators instead.

Also I can't seem to decide if find() deserves to be added as well if there is .iter().position() which is supposedly the idiomatic way to proceed. I'm not particulary for or against it, I just can't decide for myself right now. I'll think about it after work.

@zesterer
Copy link
Contributor Author

I guess there's an argument that .find() shouldn't return an index because fixed-size vectors aren't really supposed to operate upon indices. Perhaps instead it should return the value of that component?

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 27, 2018

I guess there's an argument that .find() shouldn't return an index because fixed-size vectors aren't really supposed to operate upon indices.

Why not? :) They're glorified fixed-size arrays after all.

Perhaps instead it should return the value of that component?

Then that would be v.iter().find(...).


(Sorry for the long post; it looked much less intimidating in my text editor.)

To me the question is (as you suggested), at which point plugging together existing functionality becomes unacceptable for expressing a given intent. There are multiple factors that influence my decisions, which I've always somewhat "felt" but never took the time to "formalize", if that makes sense. I'll sum there up here (and keep as notes for myself):

  • Discoverability: You've been looking for any(), but since reduce_or() didn't come up to your mind, then it's natural, as a user, to assume that it "isn't there".
    The problem is that reduce_or() didn't appear in your search results, which is expected from a machine, but not great for us as humans.
    This is the same problem as .iter().position(). I didn't know myself about it until you mentioned find(). I didn't find the answer in Rust's doc, but on StackOverflow; way to go!
  • Clarity of intent: va.map2(vb, ::std::cmp::max) is a less clear way of writing va.max(vb).
  • Convenience: v.map(|e| e.sqrt()) is clear and "atomic", but too verbose; you really just want v.sqrt(), especially since it's such a standard function.
  • Established existence: This involves all three items above. GLSL has face_forward(); users are familiar with it, and nobody questions its name or the meaning/order of arguments, so this is a no-brainer. Add it to the lib.
  • Compression (removal of redundancy): The "reduce" operation is literally { let mut i = self.into_iter(); let first = i.next().unwrap(); i.fold(first, f) }.
    Putting verbosity aside, I had to type i three times and first two times. I can't picture users or myself doing it manually every time; "make this a method already!" and I sure did.
  • Machine instruction equivalent: On SSE-enabled x86 CPUs, Vec4::<f32>::sqrt() can be performed with the SQRTPS instruction.
    So, it felt natural to add it as a first-class method instead of relying on map().

With this in mind, here's my take on your proposal:

  • Any: The way I suggested is v.map(...).reduce_or(), which happily violates discoverability, clarity of intent, and convenience (but it's an elegant way of representing the operation).
    However, there's also v.iter().any(...) which does what you want without too much fuss. The only "undesired" part is just .iter().
  • All: Same as above.
  • Find: There's v.iter().position(|x| ...) for finding the index of a value if any, and v.iter().find(|x| ...) for finding a value if any.
  • Fold: v.iter().fold(foo, |x| ...). Same as above.

Note how in all of these instances, we would just be sparing one call to .iter() or .into_iter().

Why was map() okay?
Because once you do v.iter().map(|x| ...) you have to collect() the result into another vector, meaning that you have to specify the destination type. It's also very verbose but often needed.

Why was reduce() okay?
Because the Iterator-based implementation is very verbose, and it's a very common pattern.

Why were sum() and product() okay?
Well this time I'm guilty. They're really just .iter().sum() and .iter().product() but for whatever reason I still wanted them directly as method; probably because I wanted to prepare for specialized SIMD implementations, which aren't going to happen anytime soon because they would be a huge (and not very rewarding) undertaking.


My concern is that it feels like we're trying to implement our own interface to the std, which already has acceptable solutions. If we start adding any and all, then shouldn't we also add try_fold, try_for_each, partition, and others from Iterator, even though they're already accessible with a simple call to .iter()/.into_iter()?

Conclusion: it boils down to whether or not we mind typing .iter()/.into_iter(), and which functions "deserve" to be "blessed" by being turned into inner methods.

I don't mind adding any, all and stuff, but if I do, I sure hope nobody's going to come back to me asking "hey could you please add try_fold, partition, chunks, etc etc, to the vector types?".

So, perhaps surpringly, I would rather take a conservative stance here, in order to prevent a snowball effect, and to encourage people to use the standard functions where it's reasonable enough. Thoughts? :)

@zesterer
Copy link
Contributor Author

zesterer commented Oct 27, 2018

As a suggestion... is it not possible to implement the methods of FixedSizeIterator on vector types? I see many issues with such a suggestion, but avoiding .iter() would be huge usability plus if you ask me.

@yoanlcq
Copy link
Owner

yoanlcq commented Oct 28, 2018

As a suggestion... is it not possible to implement the methods of FixedSizeIterator on vector types?

Do you mean ExactSizeIterator? All vector iterators implement it and the only methods are len() and is_empty() which are accessible without calling .iter() since vectors deref to slices.

I do agree that .iter() is just noise, which I would also rather get rid of if I could, but I think what I'm most afraid of is the inconsistencies that would result from adding inner methods.
"Well I can do v.any() or v.iter().any(). What's the difference? None? Oh, ok."
"Doesn't Vec4 implement try_rfold()? Oh right, I have to call iter() this time."

What we're really looking for is a way to get rid of the call to .iter(), which is what Deref does by sparing us calls to as_slice(). I would say "it's a job for the language or the std".

In the meantime, there could be a middle ground. Let's pretend I add any, all, none_of, find and find_index (position() could be confusing in the context of 3D apps), and fold as well.
Would that be all? Aren't there other such functions you'd like to see so I can add them all in one shot?

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

No branches or pull requests

2 participants