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

Implementing exercise binary-search #615

Closed
wants to merge 7 commits into from
Closed

Implementing exercise binary-search #615

wants to merge 7 commits into from

Conversation

samuelhnrq
Copy link

On the works!

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

What kind of Container will you use?

If it is a non continious memory store (eg. a List), the binary search is nonsense (binary search gets even worse than a linear search due to subslicing).

@samuelhnrq
Copy link
Author

Does it even matter bro? Aren't you overthinking it? it's not the point to make the better possible way to search an element in an array, rather then figuring out how to implement the algorithm, even if I knew what the heck were you talking about and indeed it were a lot slower, I don't think it would matter anyway, but that's just IMHO, what do you even mean? It's going to be slow?

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

Yes, it does matter.

The README sells the binary search as superior while it isn't for lists, so either implement the exercise in a way that it is not using a List but some kind of Array, or don't do it at all!

At least that's my opinion, but I am not a maintainer here, so at the end @exercism/haskell descides.

@Average-user
Copy link
Member

Average-user commented Oct 29, 2017

@samosaara This is supposed to be an example, therefore, the implementation matters.

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

PS: As far as I can remember the best possible implementations of binary search in a linked List ar O(n log n), but most are near to O(n²).

In worst case (element is found at the very last possible step) you'll have copied parts of the list log n times, you have to iterate each copied slice 1 to 1.5 times on naiv implementations (first half time for the element in the middle, another half time when you need left subslice, or a full time when needing the right subslice).

In a proper Array (as in a C array, meaning a continious indexed memory area), subslicing is basically a NOOP (when done correctly by the runtime) as well as indexed access does not involve itration of the full memory, but only calculating the exact point and accessing it for reading.

@Average-user
Copy link
Member

@NobbZ Sequences can do the work here right?

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

Yes, they can perform better than Lists when using their splitAt, drop, and take functions.

@samuelhnrq
Copy link
Author

samuelhnrq commented Oct 29, 2017

Oh so the whole discussion was to have the best ever, swag, 1337 h@x0r, code golf, example solution, well, ok whatever I'll use sequences. Guess I missed the point, thought you guys were complaining about the exercise itself.

@Average-user
Copy link
Member

@samosaara It isn't about having the best ever, thats impossible. The idea is to show when is useful to actually use a binary search algorithm, and apparently with lists it isn't.

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

I'm fine with the exercise, as long as the binary search is used with a correct datatype to not suggest bad practices and sell them as a good one.

@petertseng
Copy link
Member

petertseng commented Oct 29, 2017

I thought https://hackage.haskell.org/package/array-0.5.2.0/docs/Data-Array.html would have been sufficient. Wouldn't O(1) access be more suited to showing binary search than O(log(min(i, n-i))) that I see in https://hackage.haskell.org/package/containers-0.5.10.2/docs/Data-Sequence.html#v:index ? Or is even that sufficient? I guess since there would be at most O(log n) bisections one would call the runtime O(log^2 n), so you are saying that is good enough, right? Even if it is good enough, would it still be better to use an Array? If I miss something, please educate me, thank you.

@Average-user
Copy link
Member

@petertseng I just said sequences because it is what I've used. Probably arrays work too, but I can't find where it says that the access is constant.

@NobbZ
Copy link
Member

NobbZ commented Oct 29, 2017

If Arrays are O(1) in random reading and also can be easily sliced, then of course they are perfect for this algorithm, but I can't find a slicing accessor in the docs…

@petertseng
Copy link
Member

I can't find where it says that the access is constant.
If Arrays are O(1) in random reading

Okay, y'all are right, https://hackage.haskell.org/package/array-0.5.2.0/docs/Data-Array.html only specifies that "a programmer may reasonably expect rapid access to the components" without specifically explaining how rapid is the rapid referred to.

and also can be easily sliced

Shrug. Can just keep track of the left/right index of the subarray you're searching, and thereby avoid a need to slice, just keep the same array throughout.

@samuelhnrq
Copy link
Author

Okay, so, I guess I'm not that proficient in Haskell to implement with the kinda of quality you guys are requesting, for the example, so I guess I would need help (accepting PR on my fork XD) or at least time to implement this 😅

@Average-user
Copy link
Member

Average-user commented Oct 29, 2017

@samosaara I think that you have time. And the only "challenge" here, it is to get used to Data.Array.

For example you can try to implement the algorithm in Lists, as your previous intentions were. And then translate it, keeping in mind the differences between the two containers.

@samuelhnrq
Copy link
Author

Oh wait but just to be sure, I should do change the signature of the exercise itself or just convert back and forth in the example implementation?

@Average-user
Copy link
Member

@samosaara Probably the first, so that the users that are going to implement this, will be forced to use a container in which this is efficient.

@@ -0,0 +1 @@
resolver: lts-8.21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before I forget: I moved to lts-9.11 now so this should change too

@samuelhnrq
Copy link
Author

Well, there is a problem, I cant implement one of the tests because there is no way to represent an empty Data.Array, Do I ignore it? What should I do?

@petertseng
Copy link
Member

I was able to find http://mail.haskell.org/pipermail/haskell-cafe/2004-October/007098.html by asking my preferred search engine about empty arrays, but I can't say whether this information is still up to date since it is ten years old. Is it in fact out of date?

, value = 13
, expected = Nothing
}
, Case { description = "nothing is included in an empty array"
, input = []
, input = listArray (1, 0) [] -- There is no way to represent an empty array
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the dumb comment, there is. it is working

@samuelhnrq
Copy link
Author

samuelhnrq commented Nov 5, 2017

Actually I have doubts if I got that right, I didn't find a way to split the array itself, and the way I'm doing it there is not much of a point, because I'm just making it into lists so I can split them... Which was the whole point of this discussion, that doing that was slow. Did I miss a method or that's the way to do it?

@NobbZ
Copy link
Member

NobbZ commented Nov 5, 2017

In general (so regardless of the language) these algorithms avoid to copy the array. The best way to do this is to pass it around as is, and provide 2 additional parameters which determine upper and lowerbound of the active subslice.

find xs e = go (lowerIdx xs) (upperIdx xs)
  where
    go l u | xs[(l + u) / 2] == e = True
           | xs[(l + u) / 2] <  e = go (1 + (l + u) / 2) u
           | otherwise            = go l (-1 + (l + u) / 2)

This is only a rough draft and psedo code using C-like array accessors…

But it should show the overall concept

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on the array without copying.

Here's a question that I couldn't find answered in the code currently.

I saw https://github.com/exercism/problem-specifications/blob/4599c2ecbd5ff03d098b4a6eaaea0ab7dbc518db/exercises/binary-search/canonical-data.json#L73-L81 case, where you would want to have Nothing of searching for 7 in array1. I think that case should be added to the tests as well.



data Case a = Case { description :: String
, input :: Array Int a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would indent this line two spaces further so that the comma lines up with the open bracket on the previous row.


data Case a = Case { description :: String
, input :: Array Int a
, value :: a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would indent this line two spaces further so that the comma lines up with the open bracket on the previous row.

same

data Case a = Case { description :: String
, input :: Array Int a
, value :: a
, expected :: Maybe Int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would indent this line two spaces further so that the comma lines up with the open bracket on the previous row.

same

, input :: Array Int a
, value :: a
, expected :: Maybe Int
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would indent this line two spaces further so that the comma lines up with the open bracket on the previous row.

same except now I'm talking about the closing bracket

, expected = Nothing
}
]
where array1 = listArray (0, 6) [1, 3, 4, 6, 8, 9, 11]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering whether this should go above the cases so that I can see it before I see the cases it's going to be used in. I'm not sure, so let me know what you think about it.


## Source

Wikipedia [https://en.wikipedia.org/wiki/Isogram](https://en.wikipedia.org/wiki/Isogram)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isogram?

@sshine
Copy link
Contributor

sshine commented Mar 8, 2019

Hello @samosaara! I went and improved on this solution in #807, since it was so close to being finished.

A few complexities have been added to the track since you started this PR, so I've addressed those.

Thanks for doing all the groundwork.

@sshine sshine closed this in 8787e59 Mar 12, 2019
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 this pull request may close these issues.

5 participants