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

Nourhan - Spruce - C16 #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Nourhan21
Copy link

Heaps Practice

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How is a Heap different from a Binary Search Tree?
Could you build a heap with linked nodes?
Why is adding a node to a heap an O(log n) operation?
Were the heap_up & heap_down methods useful? Why?

@anselrognlie anselrognlie self-requested a review July 23, 2022 01:45
Copy link

@anselrognlie anselrognlie 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 your implementation, Nourhan. I left some comments below.

You left the comprehension questions blank, so I'm grading this as a yellow for now. But please feel free to resubmit if you would like a green score!

🟡

Comment on lines +24 to +25
Time Complexity: O(logn)
Space complexity: O(logn) because of the call stack

Choose a reason for hiding this comment

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

✨ Great. You're exactly right that it's due to the recursive call in heap_up that the space complexity is O(log n). If heap_up were implemented iteratively, this would only require O(1) space complexity since the stack size wouldn't depend on the heap depth.

"""
pass
self.store.append(HeapNode(key, value or key))

Choose a reason for hiding this comment

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

👀 To explicitly handle the case where the value is absent, prefer an explicit check with None

        if value is None:
            value = key

Using or would treat any falsy value as being missing, so we could not store False as the value, for example.

Comment on lines +33 to +34
Time Complexity: O(logn)
Space complexity: O(logn) because of the call stack

Choose a reason for hiding this comment

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

✨ Nice. Just as for add, you're right that the log space complexity remove is due to the recursive heap_down implementation. We could achieve O(1) space complexity if we used an iterative approach.

"""
pass
if self.empty():

Choose a reason for hiding this comment

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

✨ Nice use of your own helper method!

Comment on lines +38 to +41
if len(self.store) == 1:
return self.store.pop().value
minimum = self.store[0]
self.store[0] = self.store.pop()

Choose a reason for hiding this comment

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

We could avoid this special case by swapping the first element with the last (moves the minimum to the end, and a larger value to the head), then popping from the end. If there were only one thing left, it would be swapped with itself, then removed.

Comment on lines +72 to +73
Time complexity: O(logn)
Space complexity: O(logn) because of the call stack

Choose a reason for hiding this comment

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

✨ Yes, this function is where the complexity in add comes from.

Comment on lines +89 to +98
def get_valid_index_or_none(index):
if index < len(self.store):
return index
else:
return None

def get_key_or_none(index):
if not index:
return None
return self.store[index].key

Choose a reason for hiding this comment

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

✨ Nice helpers. They don't really need to be defined locally to the heap_down function, but that does prevent them from being called from anywhere else. On the one hand I like the protection, on the other hand, local functions can be confusing since they make the reader think about why the function as declared locally. Not really a right or wrong way here, just be sure to follow the style in use by the rest of your team.

left_key = get_key_or_none(left_index)
right_key = get_key_or_none(right_index)

index_to_swap = min((node for node in [(left_index, left_key), (right_index, right_key)] if node[1] is not None ), key=operator.itemgetter(1), default=None)

Choose a reason for hiding this comment

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

This is a really concise set of steps to filter out the invalid children, then select the minimum! However, it's a bit dense, which makes it a little tough to understand. Consider adding a comment about what it does, break it up into several lines with the intermediate products given descriptive names, or move into a helper function with a descriptive name.

An example of splitting across multiple lines

        candidate_children = [(left_index, left_key), (right_index, right_key)]  # we could use a tuple rather than a list
        valid_children = (node for node in candidate_children if node[1] is not None)  # this actually makes a generator function, not a tuple
        index_to_swap = min(valid_children, key=operator.itemgetter(1), default=None) 

Comment on lines +5 to +6
Time Complexity: O(nlogn)
Space Complexity: O(n)

Choose a reason for hiding this comment

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

✨ Great. Since sorting using a heap reduces down to building up a heap of n items one-by-one (each taking O(log n)), then pulling them back out again (again taking O(log n) for each of n items), we end up with a time complexity of O(2n log n) → O(n log n). While for the space, we do need to worry about the O(log n) space consume during each add and remove, but they aren't cumulative (each is consumed only during the call to add or remove). However, the internal store for the MinHeap does grow with the size of the input list. So the maximum space would be O(n + log n) → O(n), since n is a larger term than log n.

Note that a fully in-place solution (O(1) space complexity) would require both avoiding the recursive calls, as well as working directly with the originally provided list (no internal store).

for item in list:
heap.add(item)
for i in range(len(list)):

Choose a reason for hiding this comment

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

In this situation, since we built the heap, we also "know" the number of items in the heap. So it works to iterate using knowledge about the list. But if we were pulling things out of a heap more generally, we would want to make use of the empty helper as follows:

    sorted = []
    while not heap.empty():
        sorted.append(heap.remove())

    return sorted

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.

2 participants