| > From the article it clearly is meant to return [0,5,9,11,16], your rust returns [4, 8, 10, 15]. You're right, I didn't test the code, nor review it especially closely. I took a quick glance for a couple of seconds, checked that it compiled, glanced at the primitives it was using, and said "LGTM". I wanted to know "What general shape will this algorithm take expressed a different way?" You shouldn't expect perfection from someone of any skill level posting or discussing topics casually like this. The amount of time and effort I'm willing to spend on comments is limited – limited enough to allow an off-by-one error to slip through. This is commentary in an Internet forum, not a code review for a production system, or a formal publication. However, I think the nature of the error and its fix actually reinforces my point. If one did review the Rust code closely (which I didn't – I was more interested in GPT's capability to explain the APL syntax; and what the algorithm would look like expressed in another language), then a mistake would have stood out: .filter_map(move |(i, window)| {
if window == &[0, 1] {
Some(i)
If we want to return the index of the `1`, not the `0`, then `i` is the wrong index to return. I assume that's the mistake you're pointing out.Yes, it's true that I didn't even realize that the original algorithm was returning the second index of these elements and not the first. I barely skimmed those details before becoming entirely distracted by APL's syntax itself, and then started to wonder about the various other ways that such an algorithm might be implemented in another language (and how clearly, subjectively). "The APL code is opaque. Will alternatives be opaque?" Yes, a mistake like this would be trivially caught upon a close review of the algorithm or code, or on any unit testing. However, I think it bolsters my point that the correction to the Rust code is easy to understand – both the mistake and the nature of the fix: .filter_map(move |(i, window)| {
if window == &[0, 1] {
Some(i + 1)
(The only thing that's not immediately obvious is why `i+1` is guaranteed to be a valid index – which it is.)[Edit: Now that I've had time to actually run and test the code, a second error is that it omits the first group of `1`s. A corrected version is: fn find_zero_one_indices<I>(input: I) -> Vec<usize>
where
I: IntoIterator<Item = i32>,
{
let input: Vec<_> = std::iter::once(0).chain(input.into_iter()).collect();
input.windows(2)
.enumerate()
.filter_map(|(i, window)| {
if window == &[0, 1] {
Some(i)
} else {
None
}
})
.collect()
}
This returns `Some(i)` again rather than `Some(i+1)` because we've prepended a `0` to the input.]My comment is not about Rust specifically. It's just the language I chose. I assume the translation would be roughly similar in Java (with iterators), Python, Ruby, and the imperative version in C and C++ and similar. And I believe in most of these languages, the mistake and its correction would both be easy to see and understand with close enough review. I'm not certain that would be true for the APL code. If you gave me that algorithm in any mainstream language, and asked me to look for mistakes, I could probably spot that problem upon reviewing it, and probably know how to fix it. (Probably, not necessarily.) I do not believe that I could spot an analogous problem in the APL code nor understand easily how to fix it. My overall point is that the APL code seems entirely unapproachable to the un-initiated – even for a programmer experienced with numerous other languages. (I might have similar difficulty with Haskell, but probably not as much; and perhaps Scala if it was written in a particularly obscure way.) ---- An additional topic that I would be interested to explore is: what kind of errors would be likely in the APL code? What would the APL code look like that had a similar off-by-one error? Would it be easy to spot by reading the code? And what would the "diff" look like correcting it? I unfortunately don't know enough about APL to explore this. ---- [In response to several edits of your comment that contained ad hominem attacks, and a lack of explanation of your criticism:] Please follow the Hacker News guidelines: https://news.ycombinator.com/newsguidelines.html > Be kind. Don't be snarky. Converse curiously; don't cross-examine. Edit out swipes. > Comments should get more thoughtful and substantive, not less, as a topic gets more divisive. > When disagreeing, please reply to the argument instead of calling names. "That is idiotic; 1 + 1 is 2, not 3" can be shortened to "1 + 1 is 2, not 3." |
It kinda does have an off by one error; (Dyalog) APL defaults to indexing from one and the code assumes indexing from zero. The blog author isa well known APL-er and other “modern APL” users often assume indexing from zero. And the change/fix is the quad-io in my other comment so it’s not easily visible in the code because it’s not in the code at all but it is an easy fix.
At least, that’s one kind of off-by-one error; APL doing whole array transforms in each operation tends not to have as much room for off-by-one errors when you aren’t counting through things in the typical imperative way.