3
\$\begingroup\$

I built an Order Statistics tree on top of a Treap in order to solve this HackerRank problem. It works (passed all test cases). Any comments whatsoever would be greatly appreciated.

class EmptyTreeError(Exception): def __init__(self): self.error = "Empty tree" class node(object): def __init__(self,val,prty,prnt=None,lchild=None,rchild=None): self.prnt = prnt self.lchild = lchild self.rchild = rchild self.val = val self.prty = prty self.size = 1 def __str__(self, *args, **kwargs): return str((self.val,self.prty,self.size,(self.prnt.val,self.prnt.prty) if self.prnt else None)) # size needs to be recalculated all the way up to the root on insertions and deletions def recalc_size(self): self.size = (self.lchild.size if self.lchild else 0) + (self.rchild.size if self.rchild else 0) + 1 # always need to keep track of the root because it potentially changes def find_root(self): p = self while p.prnt != None: p = p.prnt return p def ppprint(self): return self.pprint([],True,[]) # basically an inorder traversal # really clever def pprint(self,pref,isTail,sb): if self.rchild: t = pref + list("│ " if isTail else " ") self.rchild.pprint(t, False, sb) t = pref + list("└── " if isTail else "┌── ") + list(str(self)) sb.append(t) if self.lchild: t = pref + list(" " if isTail else "│ ") self.lchild.pprint(t, True, sb) return sb @total_ordering class End(object): def __lt__(self,other): return False BOT = End() def rotate_left(p,r): p.rchild = r.lchild # idiot - don't forget to reset child parent if p.rchild != None: p.rchild.prnt = p r.lchild = p # need to adjust child of parent of p if p.prnt: if p == p.prnt.lchild: p.prnt.lchild = r else: p.prnt.rchild = r r.prnt = p.prnt p.prnt = r p.recalc_size() r.recalc_size() def rotate_right(p,q): p.lchild = q.rchild if p.lchild != None: p.lchild.prnt = p q.rchild = p if p.prnt: if p.prnt.lchild and p == p.prnt.lchild: p.prnt.lchild = q else: p.prnt.rchild = q q.prnt = p.prnt p.prnt = q p.recalc_size() q.recalc_size() def bubble_up(p): while p.prnt and p.prty < p.prnt.prty: if p == p.prnt.lchild: rotate_right(p.prnt,p) else: rotate_left(p.prnt,p) def bubble_down(p): while (p.lchild and p.prty > p.lchild.prty) or (p.rchild and p.prty > p.rchild.prty): if p.lchild != None and (p.rchild == None or (p.lchild.prty < p.rchild.prty)): rotate_right(p,p.lchild) elif p.rchild != None and (p.lchild == None or (p.rchild.prty < p.lchild.prty)): rotate_left(p,p.rchild) def insert(root,key): # insert and remove traversal are different: insert needs to # keep going but remove needs to stop p = root while (p.lchild and key <= p.val) or (p.rchild and key > p.val): if p.lchild and key <= p.val: p = p.lchild else: p = p.rchild if key <= p.val: nod = node(key,myRand.uniform(0,1),p) p.lchild = nod else: nod = node(key,myRand.uniform(0,1),p) p.rchild = nod # recalculate tree decorations all the way up to the root p = nod while p != None: # find root p.recalc_size() p = p.prnt bubble_up(nod) def delete(p): p.prty = BOT bubble_down(p) if p.prnt: if p.prnt.lchild and p == p.prnt.lchild: p.prnt.lchild = None else: p.prnt.rchild = None while p != None: # find root p.recalc_size() p = p.prnt else: raise EmptyTreeError def select(p,i): try: rank = (p.lchild.size if p.lchild else 0) + 1 except: print(p,i) if i == rank: return p.val elif i < rank: return select(p.lchild,i) else: return select(p.rchild,i - rank) def remove(root,key): p = root while (p.lchild and key < p.val) or (p.rchild and key > p.val): if p.lchild and key < p.val: p = p.lchild else: p = p.rchild if p.val == key: delete(p) else: raise KeyError 
\$\endgroup\$
7
  • \$\begingroup\$ FWIW, I'd be tempted to solve the problem linked with a pair of heap queues split around the median. Trees tend to be slow and error prone. \$\endgroup\$ Commented Aug 16, 2016 at 16:40
  • \$\begingroup\$ @Veedrac Indeed the solution uses two Bags partitioned around the median (I'm getting time out on the last test case I'm sure because of lines 36,38 in lieu of using a heap). \$\endgroup\$ Commented Aug 17, 2016 at 15:17
  • \$\begingroup\$ That solution doesn't use heaps, which makes rebalancing \$\mathcal{O}(n)\$ instead of \$\mathcal{O}(\log n)\$. My solution uses a median heap. Fast deletion is supported by keeping a parallel Counter. Deletion only deletes from the Counter, and when reading from the heap the top element is popped if it's not in the Counter. The heap is rebuilt when half of the elements in it are deleted to prevent its size from blowing up. \$\endgroup\$ Commented Aug 17, 2016 at 20:35
  • \$\begingroup\$ FYI, your remove_exponent is way too complex: why not just writing if x == int(x): x = int(x)? Division by two is lossless for floats. \$\endgroup\$ Commented Aug 17, 2016 at 20:37
  • \$\begingroup\$ @Veedrac what about 1.50? Maybe such instances don't show up but I wasn't really paying attention to that part of the problem (I stole the remove_exponent code from here. Thank you for the link to the median heap. \$\endgroup\$ Commented Aug 17, 2016 at 22:13

1 Answer 1

3
\$\begingroup\$

Try to avoid unnecessary shortening of variable names. prnt and p should be parent, prty should probably be parity. You should also choose more descriptive variable names than p, q and r.

PEP8 recommends lower_case for variables (so is_tail instead of isTail) and PascalCase for classes (so Node instead of node). It also specifies adding a blank after a comma in argument lists like this:

def __init__(self, val, prty, prnt=None, lchild=None, rchild=None): 

Comments like # idiot - don't forget to reset child parent should be avoided, or at least modified to # reset child parent before putting code up for everyone to see.

insert(root,key) and remove(root,key) should probably be a method of Node.

Use the @property decorator for find_root:

@property def root(self): p = self while p.parent != None: p = p.parent return p 

This way you can just call node.root. However, you never seem to use this method, so delete it.

(There was also a superfluous space after None:, which I removed).

Having two methods with the same name but different number of parameters will not work, the last function defined with that name is used (unlike for example in C++). Just use default values (but don't forget that you should never use mutable types as default arguments, see e.g. here).

def pprint(self, pref=None, isTail=True, sb=None): """Use inorder traversal to pretty print the tree""" pref = [] if not pref else pref sb = [] if not sb else sb if self.rchild: t = pref + list("│ " if isTail else " ") self.rchild.pprint(t, False, sb) t = pref + list("└── " if isTail else "┌── ") + list(str(self)) sb.append(t) if self.lchild: t = pref + list(" " if isTail else "│ ") self.lchild.pprint(t, True, sb) return sb 

(I did not fix the variable names here. You should probably come up with better names than sb, though).

You should also add a docstring to your functions to explain what they do and how they are to be used.

\$\endgroup\$
2
  • \$\begingroup\$ Much appreciated. \$\endgroup\$ Commented Aug 17, 2016 at 15:19
  • \$\begingroup\$ @MaksimLevental You're welcome! Just added a bit more after having another look at your code. \$\endgroup\$ Commented Aug 18, 2016 at 7:31

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.