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

ArrayDeque added #247

Closed
wants to merge 4 commits into from
Closed

Conversation

HarsheetKakar
Copy link
Contributor

References to other Issues or PRs or Relevant literature

Fixes #120

Brief description of what is fixed or changed

ArrayDeque added
small change in DODA instead of obj[0] it is now obj._data[0] in __new__

Other comments

@codecov
Copy link

codecov bot commented Apr 5, 2020

Codecov Report

Merging #247 into master will increase coverage by 0.056%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master      #247       +/-   ##
=============================================
+ Coverage   98.575%   98.632%   +0.056%     
=============================================
  Files           24        24               
  Lines         2106      2193       +87     
=============================================
+ Hits          2076      2163       +87     
  Misses          30        30               
Impacted Files Coverage Δ
pydatastructs/linear_data_structures/arrays.py 100.000% <100.000%> (ø)
...datastructs/miscellaneous_data_structures/queue.py 100.000% <100.000%> (ø)

Impacted file tree graph

@czgdp1807
Copy link
Member

Please follow the pattern currently there in queue.py. Can we avoid adding new classes?

@HarsheetKakar
Copy link
Contributor Author

Please follow the pattern currently there in queue.py. Can we avoid adding new classes?

add it in the implementation? but the thing is deque is best of stack and queue and is implemented like DODA so I thought it doesnt fit in as another implementation of queue. I can change it if you want.

@czgdp1807
Copy link
Member

but the thing is deque is best of stack and queue and is implemented like DODA so I thought it doesnt fit in as another implementation of queue. I can change it if you want.

It should be changed. A doubly ended queue is anyway a queue. You can re-define some methods in the implementation to make things work.

@HarsheetKakar
Copy link
Contributor Author

but the thing is deque is best of stack and queue and is implemented like DODA so I thought it doesnt fit in as another implementation of queue. I can change it if you want.

It should be changed. A doubly ended queue is anyway a queue. You can re-define some methods in the implementation to make things work.

ok


@property
def is_empty(self):
return self.size == 0
Copy link
Member

Choose a reason for hiding this comment

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

May be something using, self.front and self.rear. What's there in the parent classes? What's the MRO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried using front and rear but this worked better since it is circular front can be behind rear in 2 cases if the deque is empty or if it is full and since % operator is used there is no way to find which one is the case


@property
def is_full(self):
return self.size == len(self._data)
Copy link
Member

Choose a reason for hiding this comment

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

+1

Comment on lines +468 to +474
def __getitem__(self, idx):
idx = (self.rear + idx) % len(self._data)
return self._data[idx]

def __setitem__(self, idx, val):
idx = (self.rear + idx) % len(self._data)
self._data[idx] = val
Copy link
Member

Choose a reason for hiding this comment

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

In queue, elements are accessed from end points so these methods shouldn't be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

helps a lot in rest of the implementation since I dont have to do modular arithmetic every time I push or pop

Copy link
Member

Choose a reason for hiding this comment

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

At least they shouldn't be public. You can define helper methods, though 2 line is not much for code indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and these functions are very helpful if u want to change the value of some key in the queue, the user does not have to think about rear and front they can just start from 0 and go up to self.size. It actually makes the code a lot cleaner.

Copy link
Member

@czgdp1807 czgdp1807 Apr 6, 2020

Choose a reason for hiding this comment

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

the user does not have to think about rear and front

If someone wants to change the value of a key(somewhere other than front and rear), they could have used some other data structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we dont override it here then if user uses the index it will use DODA's getitem and setitem which will break the logic, and anyway it isnt breaking the already set datastructures and it helps a lot in development also as you can see in append/ appendleft. It makes the code readable, it helps to remove unnecessary bugs and there are only 2 small methods in question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and sir it also helps other developers to understand the code.

@czgdp1807
Copy link
Member

Can we do the following,

class ArrayDeque(ArrayQueue):
   ...

@HarsheetKakar
Copy link
Contributor Author

Can we do the following,

class ArrayDeque(ArrayQueue):
   ...

Error while trying to put it as ArrayQueue
Exception has occurred: TypeError multiple bases have instance lay-out conflict

@HarsheetKakar
Copy link
Contributor Author

@czgdp1807 shall I add LinkedList implementation as well since then both of them would come under Deque abstract class and there would be no inconsistency with the already written code?

@czgdp1807
Copy link
Member

I think the classes, ArrayDeque use DODA rather than inheriting and overriding it's methods. You can add LinkedList implementation, though again it should be used as a internal data member rather than being inherited.

@HarsheetKakar
Copy link
Contributor Author

I think the classes, ArrayDeque use DODA rather than inheriting and overriding it's methods. You can add LinkedList implementation, though again it should be used as a internal data member rather than being inherited.

the append and pop method in ArrayDeque is quite different from DODA so if I have to use its properties and not its caveats I have to inherit it. I can also implement it from scratch but it will take more time.

@czgdp1807
Copy link
Member

Any news?

@HarsheetKakar
Copy link
Contributor Author

Any news?

sorry forgot about it, give me a couple of days

@HarsheetKakar
Copy link
Contributor Author

I think the classes, ArrayDeque use DODA rather than inheriting and overriding it's methods. You can add LinkedList implementation, though again it should be used as a internal data member rather than being inherited.

the append and pop method in ArrayDeque is quite different from DODA so if I have to use its properties and not its caveats I have to inherit it. I can also implement it from scratch but it will take more time.

and you havent answered this one.

@czgdp1807
Copy link
Member

Can we do append and pop as follows,

def append(self, x):
    self.queue.append(x)
    self.rear += 1
def pop(self, x):
    self.queue.delete(front)
    self.front += 1
    # check corner cases like if it's empty

@HarsheetKakar
Copy link
Contributor Author

Can we do append and pop as follows,

def append(self, x):
    self.queue.append(x)
    self.rear += 1
def pop(self, x):
    self.queue.delete(front)
    self.front += 1
    # check corner cases like if it's empty

I dont think we can since rear is moved back to 0 if the array is doubled, this helps in keeping the _modify method less complicated.

@czgdp1807
Copy link
Member

czgdp1807 commented Apr 20, 2020

Probably, keeping, rear as self._last_pos_filled and tracking front by checking when the array will be double should do the the thing.

@HarsheetKakar
Copy link
Contributor Author

Probably, keeping, rear as self._last_pos_filled and tracking front by checking when the array will be double should do the the thing.

sir but it is doubly ended so that means that rear also would move in the opposite direction, keeping track of both is important... with that said I have forgotten the implementation of most of it and would like to work on some other issue, so if its not a problem with you, we can put upforgrab on it... since most of the work is done someone can refactor the code easily

@czgdp1807 czgdp1807 added Please take over PRs that can be continued by anyone. and removed gssoc20 labels Apr 20, 2020
@czgdp1807
Copy link
Member

Closing in favor of #321

@czgdp1807 czgdp1807 closed this Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array implementation of doubly ended queue (deque)
3 participants