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

Implemented PriorityQueue and Queue in utils.py #828

Closed
wants to merge 9 commits into from
Closed

Implemented PriorityQueue and Queue in utils.py #828

wants to merge 9 commits into from

Conversation

goswami-rahul
Copy link
Contributor

@goswami-rahul goswami-rahul commented Mar 8, 2018

  • Defined the abstract methods in the Queue abstract class.
  • Implemented the PriorityQueue methods. (on the TODO list)
  • Implemented the FIFOQueue methods.
  • Changed the 2 usages of PriorityQueue to comply with new changes.
    The __init__ method of the PriorityQueue is changed.
    Old:
 def __init__(self, order=min, f=lambda x: x):
        self.A = []
        self.order = order
        self.f = f

New:

    def __init__(self, order='min', f=lambda x: x):
        self.q = []

        if order == 'min':
            self.f = f
        elif order == 'max':            # now item with max f(x)
            self.f = lambda x: -f(x)    # will be popped first
        else:
            raise ValueError("order must be either 'min' or max'.")

This change is because it can be seen that the argument passed before i.e. min is a built-in function,
There is no specific use of min function in any method. It is just used as a flag to indicate that make a min PriorityQueue.
This functionality is achieved better when the order argument is str rather than a function.
There are 2 uses of this class, both of which are also updated in this PR.
Also now the PriorityQueue attribute self.order is not required.

Any suggestions or feedbacks are appreciated.

@goswami-rahul
Copy link
Contributor Author

goswami-rahul commented Mar 8, 2018

I don't understand what the __getitems__ method is doing? I have opened an issue (#829) regarding the same.
I have not changed this method for now.

@goswami-rahul
Copy link
Contributor Author

goswami-rahul commented Mar 9, 2018

@MrDupin @norvig
The Stack() too should be a subclass of Queue basically with all the methods wrapped inside the subclass. I think defining Stack as a subclass would be programming approach.
I can also include the changes made in the Stack in this PR, but I need your suggestions.
Should I open an issue regarding this?

@antmarakis
Copy link
Collaborator

antmarakis commented Mar 10, 2018

I have previously tried to make changes to the Queues, but each time I realized that it's not a big issue. Students don't come here for this, so we should just have something simple that works.

I am not sure what to do with this, sorry. It can go any way.

For now focus on the Queues and if Mr. Norvig likes the changes, work on the Stack.

@@ -662,41 +663,39 @@ def __missing__(self, key):


class hashabledict(dict):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the motivation for a hashable dict. It is too bad Python doesn't have frozendict like frozenset.

But the implementation here is O(n) to compare or hash. Given that, a simpler representation is:

class hdict(dict):
    def __hash__(self): return 1

@@ -724,73 +739,94 @@ def Stack():


class FIFOQueue(Queue):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code here is fine, but I think we can just use collections.deque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we have to change every uses of FIFOQueue and replace its pop with popleft. I thought the idea behind these classes here was to be able to use PriorityQueue and FIFOQueue (and maybe Stack) with a the same code and just change in the constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, @goswami-rahul you are correct that the motivation of a single interface was to make the various queues the same, so that they could be interchangeable, and we could highlight the similarities. This was a priority in the 1st and 2nd edition of the book (and the code goes all the way back to then). But in the 3rd (and soon 4th) edition, we accept that each search function is somewhat different, and so it is ok, and maybe even preferably, to have popleft instead of pop.


def __contains__(self, item):
return item in self.queue
return item in self.q


class PriorityQueue(Queue):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably easier to use heapq.

@goswami-rahul
Copy link
Contributor Author

So should I remove all these classes and refactor all their instances with collections.deque.?

@goswami-rahul
Copy link
Contributor Author

new changes in #878

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.

3 participants