Included linear + binary search for 1D static arrays #454

r-avenous · 2021-12-08T12:41:51Z

References to other Issues or PRs or Relevant literature

Brief description of what is fixed or changed

A search function is added.
It has 2 necessary parameters(1. element to search, 2. order of the array) and one optional (3. comparator)

what to search
positive values (int or float allowed, anything else raises type error exception) mean ascending order, negative is descending, 0 is unsorted
function as a parameter; this is for when the usual >, < do not work on the datatype of the array

Other comments

czgdp1807 · 2021-12-08T14:19:49Z

pydatastructs/linear_data_structures/arrays.py

@@ -126,7 +126,7 @@ def __new__(cls, dtype=NoneType, *args, **kwargs):
    @classmethod
    def methods(cls):
        return ['__new__', '__getitem__',
-        '__setitem__', 'fill', '__len__']
+        '__setitem__', 'fill', '__len__', 'search']


Please make it a function (like place in algorithms.py in linear_data_structures) instead of a class method?

Pardon me, shall I include this directly? Since all this does is search in 1D arrays only so I did that. I wanted to include this for dynamic arrays too, but that removal of items just makes a void location and does not shift the items in those void locations

Regardless, I will include a jump search as it was mentioned as an idea in the issue. Again, for 1D static arrays. Is that fine?

That should be fine. I am planning to add ImplicitArray
(with same interface as explicit arrays available right now in pydatastructs) so it would be useful to have search functions for that as well. And anyways, conceptually, search is an algorithm that just reads the data from an array. It's not responsible for updating the state of OneDimensionalArray class, so having it as a method is not of much use.

Understood. So should I just move this change to algorithms.py along with addition of the jump search? Just confirming

I don't think that's a good idea. The logic for each type of array should be encapsulated in a single function. The user can decide on their own what they might want to use. Please make separate functions for binary_search, linear_search, etc.

Ok understood

Uh sorry to ask this. Should I make a new PR after doing as told?

Not needed. You can update this one by pushing new commits.

I pushed the new commit.

czgdp1807 · 2021-12-08T16:28:47Z

pydatastructs/linear_data_structures/algorithms.py

@@ -1369,3 +1372,58 @@ def bubble_sort(array, **kwargs):
        array._modify(force=True)

    return array
+
+def linear_search(array, elem):
+    for i in range(array._size):


Is array.size not working? We shouldn't access private attributes (the ones having _ as prefix) unless really needed.

no no. While seeing the code, I noticed that only _size is being used, so I used that too

czgdp1807 · 2021-12-08T16:29:14Z

pydatastructs/linear_data_structures/algorithms.py

+    L = 0
+    R = array._size - 1


left, right?

Please make it explicit then. Always use full names instead of short hand notations. Not many key strokes will be saved while typing, L instead of left.

pydatastructs/linear_data_structures/arrays.py

czgdp1807 · 2021-12-08T16:30:50Z

pydatastructs/linear_data_structures/algorithms.py

+                R = M - 1
+    return -1
+
+def jump_search(array, elem, comp = None):


You might have noticed that all the APIs in this file have start, end, backend as kwargs arguments. Is there any specific reason for not adding the same in all the APIs you have added.

No, I am afraid of making mistakes since I dont have much expertise, so I did the way I was confident in.

Sorry for coming short

Are there any issues?

Yes. All the APIs should be consistent to the maximum extent possible. Now, here, start, end and backend should be included and the implementation should be modified to use these parameters.

Well, you had to first pull the changes from forArrays branch. git pull your_remote_name forArrays. That will fetch the suggestion you applied here, then you will be able to push to your forArrays branch.

Is there still some issue? Since this is marked as a draft.

Please add tests for all your APIs. See, tests/test_algorithms.py file under, linear_data_structures.

Doc string are also to be added. See other algorithms in the same file.

Co-authored-by: Gagandeep Singh <[email protected]>

…into forArrays

pydatastructs/linear_data_structures/algorithms.py

czgdp1807 · 2021-12-09T13:36:23Z

pydatastructs/linear_data_structures/algorithms.py

+    Left = start
+    Right = end
+    while Left <= Right:
+        Middle = int((Left+Right)/2)


What about, Left//2 + Right//2?

I think (Left + Right)//2 is fine but not too sure about the sum of floors, for example, when left = 3, right = 5(when both odd, can create problems), it will give 3 as middle

Well the loss won't be much. There are four cases,

Left - Odd, Right - Odd - Middle will be lesser (by 1) in Left//2 + Right//2 as compared to current approach.

Left - Odd, Right - Even or Right - Odd, Left - Even - Middle will be exactly the same as in current approach.

Left - Even, Right - Even - Middle will again be exactly the same as in current approach.

The reason is (Left + Right) might get too large for implicit arrays leading to slow down in performance (Python uses big integers for handling larger than 64 bit integers). So, basically if you can just add 1 for Left - Odd, Right - Odd in Left//2 + Right//2 and do only Left//2 + Right//2 for other cases then it should be fine. In fact,

Middle = Left//2 + Right//2 + Left % 2 * Right % 2 should work. Worth trying.

Yes, this seems much better, I shall do this

czgdp1807 · 2021-12-09T13:39:16Z

pydatastructs/linear_data_structures/tests/test_algorithms.py

+    ODA = OneDimensionalArray
+
+    array = ODA(int, [1, 2, 5, 7, 10, 29, 40])
+    for i in range(len(array)):
+        assert i == jump_search(array, array[i])


What about corner cases like, empty array, one element array? And see _test_common_sort above. Repetition of tests can be avoided by doing something similar here.

Are you asking for the test function or the actual algorithm written?

Yes. You are using the same test for every search algorithm. So why not write a common test function, _test_common_search(search_function_api) and then in test_jump_search (for example),

def test_jump_search(): _test_common_search(jump_search)

Right, I will do this

codecov · 2021-12-09T13:40:52Z

Codecov Report

Merging #454 (1a3439b) into master (f82f52d) will increase coverage by 0.015%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master      #454       +/-   ##
=============================================
+ Coverage   98.588%   98.603%   +0.015%     
=============================================
  Files           29        29               
  Lines         3754      3795       +41     
=============================================
+ Hits          3701      3742       +41     
  Misses          53        53

Impacted Files	Coverage Δ
pydatastructs/linear_data_structures/__init__.py	`100.000% <ø> (ø)`
pydatastructs/linear_data_structures/algorithms.py	`99.765% <100.000%> (+0.024%)`	⬆️

czgdp1807 · 2021-12-09T13:57:50Z

See this. The red lines are not covered. Add tests for covering them as well.

r-avenous · 2021-12-09T14:17:12Z

See this. The red lines are not covered. Add tests for covering them as well.

Its showing that bubble sort is not covered, but it is, I did not touch anything other than searching which I added

czgdp1807 · 2021-12-09T14:19:10Z

Its showing that bubble sort is not covered, but it is, I did not touch anything other than searching which I added

That's the red (lighter shade) of diff. If you scroll and click on Click to load diff then you will see dark red lines. These are the ones that you have to cover by adding tests.

Co-authored-by: Gagandeep Singh <[email protected]>

…into forArrays

r-avenous · 2021-12-09T14:26:11Z

Its showing that bubble sort is not covered, but it is, I did not touch anything other than searching which I added

That's the red (lighter shade) of diff. If you scroll and click on Click to load diff then you will see dark red lines. These are the ones that you have to cover by adding tests.

Ok on it. But still, bubble sort and another line of importing is in deep red, what about it?

pydatastructs/linear_data_structures/tests/test_algorithms.py

Co-authored-by: Gagandeep Singh <[email protected]>

czgdp1807 · 2021-12-09T14:37:54Z

bubble sort and another line of importing is in deep red, what about it?

Ignore. Though you can generate the same report locally if Codecov UI is confusing. See the testing section of README. Once you execute the command there, you will have to open the index.html file in html/cov in a browser.

r-avenous · 2021-12-09T14:42:35Z

bubble sort and another line of importing is in deep red, what about it?

Ignore. Though you can generate the same report locally if Codecov UI is confusing. See the testing section of README. Once you execute the command there, you will have to open the index.html file in html/cov in a browser.

Can I use Codecov? I am comfortable with it

czgdp1807 · 2021-12-09T14:43:50Z

Sure.

r-avenous · 2021-12-09T14:46:31Z

Sure.

Is there any other issue?

czgdp1807 · 2021-12-09T14:54:05Z

I will push some changes to your branch forArrays may be on Sunday. I don't think there is anything else to be done from your side.

r-avenous · 2021-12-09T14:56:37Z

I will push some changes to your branch forArrays may be on Sunday. I don't think there is anything else to be done from your side.

Please let me know if there is any error. Shall I see if I can contribute something else in the project?

czgdp1807 · 2021-12-09T14:59:57Z

Shall I see if I can contribute something else in the project?

Sure go ahead.

Pushed by me.

czgdp1807 · 2021-12-12T13:19:02Z

Thanks @skullVoid

r-avenous · 2021-12-12T13:38:39Z

Thanks @skullVoid

I didnt do much. Thank you for guiding me, this was a good experience for me as a first timer.

linear+binarySearch

d0c7ed3

czgdp1807 reviewed Dec 8, 2021

View reviewed changes

czgdp1807 added algorithms linear_data_structures linear_data_structures.algorithms labels Dec 8, 2021

searchingFor1D_arrays

f7e0f51

czgdp1807 previously requested changes Dec 8, 2021

View reviewed changes

r-avenous and others added 2 commits December 9, 2021 00:11

Update pydatastructs/linear_data_structures/arrays.py

e3f08c5

Co-authored-by: Gagandeep Singh <[email protected]>

searchingFor1D_array

48cbcb0

r-avenous closed this Dec 9, 2021

r-avenous deleted the forArrays branch December 9, 2021 03:33

r-avenous restored the forArrays branch December 9, 2021 06:12

r-avenous reopened this Dec 9, 2021

Merge branch 'forArrays' of https://github.com/skullVoid/pydatastructs …

a10a871

…into forArrays

czgdp1807 marked this pull request as draft December 9, 2021 07:06

Added comments, tested, for 3 search algorithms

1388f78

r-avenous requested a review from czgdp1807 December 9, 2021 13:21