Python program to solve the “skyline problem”Why is this divide and conquer algorithm inefficient?Programming Challenge: Drawing ToolCodeEval's SkyScrapers challengeSkyline problem solved by sweep lineFind number of neighbour pairs in the input treeBFS shortest path for Google Foobar “Prepare the Bunnies' Escape”Leetcode 317: Shortest distance from all buildingsCodeFights: Pipes gameMax Increase to Keep City Skyline in JavaPython program to solve the “rod-cutting problem”

When conversion from Integer to Single may lose precision

Subtables with equal width?

What LISP compilers and interpreters were available for 8-bit machines?

Does the growth of home value benefit from compound interest?

Can an Eldritch Knight use Action Surge and thus Arcane Charge even when surprised?

4*4*4 Rubiks cube Top Layer Issue

Java guess the number

Did thousands of women die every year due to illegal abortions before Roe v. Wade?

Can you really not move between grapples/shoves?

SF novella separating the dumb majority from the intelligent part of mankind

Do any instruments not produce overtones?

Strat tremolo bar has tightening issues

Bent spoke design wheels — feasible?

Etymology of 'calcit(r)are'?

Remove sudoers using script

Avoiding cliches when writing gods

Proof that shortest path with negative cycles is NP hard

Can characters escape from Death House through this method?

How bad would a partial hash leak be, realistically?

What do we gain with higher order logics?

Building a road to escape Earth's gravity by making a pyramid on Antartica

What is the advantage of carrying a tripod and ND-filters when you could use image stacking instead?

How to skip replacing first occurrence of a character in each line?

Translating 'Liber'



Python program to solve the “skyline problem”


Why is this divide and conquer algorithm inefficient?Programming Challenge: Drawing ToolCodeEval's SkyScrapers challengeSkyline problem solved by sweep lineFind number of neighbour pairs in the input treeBFS shortest path for Google Foobar “Prepare the Bunnies' Escape”Leetcode 317: Shortest distance from all buildingsCodeFights: Pipes gameMax Increase to Keep City Skyline in JavaPython program to solve the “rod-cutting problem”






.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;








5












$begingroup$


This is a Leetcode problem -




A city's skyline is the outer contour of the silhouette formed by all
the buildings in that city when viewed from a distance. Now suppose
you are given the locations and height of all the buildings as
shown on a cityscape photo (Figure A), write a program to output the
skyline
formed by these buildings collectively (Figure B).



enter image description here



enter image description here



The geometric information of each building is represented by a triplet
of integers [Li, Ri, Hi], where Li and Ri are the x
coordinates of the left and right edge of the ith building,
respectively, and Hi is its height. It is guaranteed that 0 ≤ Li,
Ri ≤ INT_MAX
, 0 < Hi ≤ INT_MAX, and Ri - Li > 0. You may assume
all buildings are perfect rectangles grounded on an absolutely flat
surface at height 0.



For instance, the dimensions of all buildings in Figure A are recorded
as: [[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24,
8]]
.



The output is a list of "key points" (red dots in Figure B) in the
format of [[x1,y1], [x2, y2], [x3, y3], ...] that uniquely defines a
skyline. A key point is the left endpoint of a horizontal line
segment
. Note that the last key point, where the rightmost building
ends, is merely used to mark the termination of the skyline, and
always has zero height. Also, the ground in between any two adjacent
buildings should be considered part of the skyline contour.



For instance, the skyline in Figure B should be represented as: [[2,
10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]
.



Notes -



The number of buildings in any input list is guaranteed to be in the
range [0, 10000].



The input list is already sorted in ascending order by the left x
position Li.



The output list must be sorted by the x position.



There must be no consecutive horizontal lines of equal height in the
output skyline. For instance, [...[2 3], [4 5], [7 5], [11 5], [12
7]...]
is not acceptable; the three lines of height 5 should be
merged into one in the final output as such: [...[2 3], [4 5], [12
7], ...]
.




Here is my solution to this task using divide and conquer (in Python) -




 class Solution:
def get_skyline(self, buildings):
"""
:type buildings: List[List[int]]
:rtype: List[List[int]]
"""
if not buildings:
return []
if len(buildings) == 1:
return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]

mid = len(buildings) // 2
left = self.get_skyline(buildings[:mid])
right = self.get_skyline(buildings[mid:])
return self.merge(left, right)

def merge(self, left, right):
h1, h2 = 0, 0
i, j = 0, 0
result = []

while i < len(left) and j < len(right):
if left[i][0] < right[j][0]:
h1 = left[i][1]
corner = left[i][0]
i += 1
elif right[j][0] < left[i][0]:
h2 = right[j][1]
corner = right[j][0]
j += 1
else:
h1 = left[i][1]
h2 = right[j][1]
corner = right[j][0]
i += 1
j += 1
if self.is_valid(result, max(h1, h2)):
result.append([corner, max(h1, h2)])
result.extend(right[j:])
result.extend(left[i:])
return result

def is_valid(self, result, new_height):
return not result or result[-1][1] != new_height



Here is an example output -



#output = Solution()
#print(output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]]))

>>> [[2, 10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]


Here is the time taken for this output -



%timeit output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]])
>>> 27.6 µs ± 3.65 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)


So, I would like to know whether I could make this program more efficient and/or shorter. Any alternatives are welcome.










share|improve this question











$endgroup$


















    5












    $begingroup$


    This is a Leetcode problem -




    A city's skyline is the outer contour of the silhouette formed by all
    the buildings in that city when viewed from a distance. Now suppose
    you are given the locations and height of all the buildings as
    shown on a cityscape photo (Figure A), write a program to output the
    skyline
    formed by these buildings collectively (Figure B).



    enter image description here



    enter image description here



    The geometric information of each building is represented by a triplet
    of integers [Li, Ri, Hi], where Li and Ri are the x
    coordinates of the left and right edge of the ith building,
    respectively, and Hi is its height. It is guaranteed that 0 ≤ Li,
    Ri ≤ INT_MAX
    , 0 < Hi ≤ INT_MAX, and Ri - Li > 0. You may assume
    all buildings are perfect rectangles grounded on an absolutely flat
    surface at height 0.



    For instance, the dimensions of all buildings in Figure A are recorded
    as: [[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24,
    8]]
    .



    The output is a list of "key points" (red dots in Figure B) in the
    format of [[x1,y1], [x2, y2], [x3, y3], ...] that uniquely defines a
    skyline. A key point is the left endpoint of a horizontal line
    segment
    . Note that the last key point, where the rightmost building
    ends, is merely used to mark the termination of the skyline, and
    always has zero height. Also, the ground in between any two adjacent
    buildings should be considered part of the skyline contour.



    For instance, the skyline in Figure B should be represented as: [[2,
    10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]
    .



    Notes -



    The number of buildings in any input list is guaranteed to be in the
    range [0, 10000].



    The input list is already sorted in ascending order by the left x
    position Li.



    The output list must be sorted by the x position.



    There must be no consecutive horizontal lines of equal height in the
    output skyline. For instance, [...[2 3], [4 5], [7 5], [11 5], [12
    7]...]
    is not acceptable; the three lines of height 5 should be
    merged into one in the final output as such: [...[2 3], [4 5], [12
    7], ...]
    .




    Here is my solution to this task using divide and conquer (in Python) -




     class Solution:
    def get_skyline(self, buildings):
    """
    :type buildings: List[List[int]]
    :rtype: List[List[int]]
    """
    if not buildings:
    return []
    if len(buildings) == 1:
    return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]

    mid = len(buildings) // 2
    left = self.get_skyline(buildings[:mid])
    right = self.get_skyline(buildings[mid:])
    return self.merge(left, right)

    def merge(self, left, right):
    h1, h2 = 0, 0
    i, j = 0, 0
    result = []

    while i < len(left) and j < len(right):
    if left[i][0] < right[j][0]:
    h1 = left[i][1]
    corner = left[i][0]
    i += 1
    elif right[j][0] < left[i][0]:
    h2 = right[j][1]
    corner = right[j][0]
    j += 1
    else:
    h1 = left[i][1]
    h2 = right[j][1]
    corner = right[j][0]
    i += 1
    j += 1
    if self.is_valid(result, max(h1, h2)):
    result.append([corner, max(h1, h2)])
    result.extend(right[j:])
    result.extend(left[i:])
    return result

    def is_valid(self, result, new_height):
    return not result or result[-1][1] != new_height



    Here is an example output -



    #output = Solution()
    #print(output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]]))

    >>> [[2, 10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]


    Here is the time taken for this output -



    %timeit output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]])
    >>> 27.6 µs ± 3.65 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)


    So, I would like to know whether I could make this program more efficient and/or shorter. Any alternatives are welcome.










    share|improve this question











    $endgroup$














      5












      5








      5


      1



      $begingroup$


      This is a Leetcode problem -




      A city's skyline is the outer contour of the silhouette formed by all
      the buildings in that city when viewed from a distance. Now suppose
      you are given the locations and height of all the buildings as
      shown on a cityscape photo (Figure A), write a program to output the
      skyline
      formed by these buildings collectively (Figure B).



      enter image description here



      enter image description here



      The geometric information of each building is represented by a triplet
      of integers [Li, Ri, Hi], where Li and Ri are the x
      coordinates of the left and right edge of the ith building,
      respectively, and Hi is its height. It is guaranteed that 0 ≤ Li,
      Ri ≤ INT_MAX
      , 0 < Hi ≤ INT_MAX, and Ri - Li > 0. You may assume
      all buildings are perfect rectangles grounded on an absolutely flat
      surface at height 0.



      For instance, the dimensions of all buildings in Figure A are recorded
      as: [[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24,
      8]]
      .



      The output is a list of "key points" (red dots in Figure B) in the
      format of [[x1,y1], [x2, y2], [x3, y3], ...] that uniquely defines a
      skyline. A key point is the left endpoint of a horizontal line
      segment
      . Note that the last key point, where the rightmost building
      ends, is merely used to mark the termination of the skyline, and
      always has zero height. Also, the ground in between any two adjacent
      buildings should be considered part of the skyline contour.



      For instance, the skyline in Figure B should be represented as: [[2,
      10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]
      .



      Notes -



      The number of buildings in any input list is guaranteed to be in the
      range [0, 10000].



      The input list is already sorted in ascending order by the left x
      position Li.



      The output list must be sorted by the x position.



      There must be no consecutive horizontal lines of equal height in the
      output skyline. For instance, [...[2 3], [4 5], [7 5], [11 5], [12
      7]...]
      is not acceptable; the three lines of height 5 should be
      merged into one in the final output as such: [...[2 3], [4 5], [12
      7], ...]
      .




      Here is my solution to this task using divide and conquer (in Python) -




       class Solution:
      def get_skyline(self, buildings):
      """
      :type buildings: List[List[int]]
      :rtype: List[List[int]]
      """
      if not buildings:
      return []
      if len(buildings) == 1:
      return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]

      mid = len(buildings) // 2
      left = self.get_skyline(buildings[:mid])
      right = self.get_skyline(buildings[mid:])
      return self.merge(left, right)

      def merge(self, left, right):
      h1, h2 = 0, 0
      i, j = 0, 0
      result = []

      while i < len(left) and j < len(right):
      if left[i][0] < right[j][0]:
      h1 = left[i][1]
      corner = left[i][0]
      i += 1
      elif right[j][0] < left[i][0]:
      h2 = right[j][1]
      corner = right[j][0]
      j += 1
      else:
      h1 = left[i][1]
      h2 = right[j][1]
      corner = right[j][0]
      i += 1
      j += 1
      if self.is_valid(result, max(h1, h2)):
      result.append([corner, max(h1, h2)])
      result.extend(right[j:])
      result.extend(left[i:])
      return result

      def is_valid(self, result, new_height):
      return not result or result[-1][1] != new_height



      Here is an example output -



      #output = Solution()
      #print(output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]]))

      >>> [[2, 10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]


      Here is the time taken for this output -



      %timeit output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]])
      >>> 27.6 µs ± 3.65 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)


      So, I would like to know whether I could make this program more efficient and/or shorter. Any alternatives are welcome.










      share|improve this question











      $endgroup$




      This is a Leetcode problem -




      A city's skyline is the outer contour of the silhouette formed by all
      the buildings in that city when viewed from a distance. Now suppose
      you are given the locations and height of all the buildings as
      shown on a cityscape photo (Figure A), write a program to output the
      skyline
      formed by these buildings collectively (Figure B).



      enter image description here



      enter image description here



      The geometric information of each building is represented by a triplet
      of integers [Li, Ri, Hi], where Li and Ri are the x
      coordinates of the left and right edge of the ith building,
      respectively, and Hi is its height. It is guaranteed that 0 ≤ Li,
      Ri ≤ INT_MAX
      , 0 < Hi ≤ INT_MAX, and Ri - Li > 0. You may assume
      all buildings are perfect rectangles grounded on an absolutely flat
      surface at height 0.



      For instance, the dimensions of all buildings in Figure A are recorded
      as: [[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24,
      8]]
      .



      The output is a list of "key points" (red dots in Figure B) in the
      format of [[x1,y1], [x2, y2], [x3, y3], ...] that uniquely defines a
      skyline. A key point is the left endpoint of a horizontal line
      segment
      . Note that the last key point, where the rightmost building
      ends, is merely used to mark the termination of the skyline, and
      always has zero height. Also, the ground in between any two adjacent
      buildings should be considered part of the skyline contour.



      For instance, the skyline in Figure B should be represented as: [[2,
      10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]
      .



      Notes -



      The number of buildings in any input list is guaranteed to be in the
      range [0, 10000].



      The input list is already sorted in ascending order by the left x
      position Li.



      The output list must be sorted by the x position.



      There must be no consecutive horizontal lines of equal height in the
      output skyline. For instance, [...[2 3], [4 5], [7 5], [11 5], [12
      7]...]
      is not acceptable; the three lines of height 5 should be
      merged into one in the final output as such: [...[2 3], [4 5], [12
      7], ...]
      .




      Here is my solution to this task using divide and conquer (in Python) -




       class Solution:
      def get_skyline(self, buildings):
      """
      :type buildings: List[List[int]]
      :rtype: List[List[int]]
      """
      if not buildings:
      return []
      if len(buildings) == 1:
      return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]

      mid = len(buildings) // 2
      left = self.get_skyline(buildings[:mid])
      right = self.get_skyline(buildings[mid:])
      return self.merge(left, right)

      def merge(self, left, right):
      h1, h2 = 0, 0
      i, j = 0, 0
      result = []

      while i < len(left) and j < len(right):
      if left[i][0] < right[j][0]:
      h1 = left[i][1]
      corner = left[i][0]
      i += 1
      elif right[j][0] < left[i][0]:
      h2 = right[j][1]
      corner = right[j][0]
      j += 1
      else:
      h1 = left[i][1]
      h2 = right[j][1]
      corner = right[j][0]
      i += 1
      j += 1
      if self.is_valid(result, max(h1, h2)):
      result.append([corner, max(h1, h2)])
      result.extend(right[j:])
      result.extend(left[i:])
      return result

      def is_valid(self, result, new_height):
      return not result or result[-1][1] != new_height



      Here is an example output -



      #output = Solution()
      #print(output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]]))

      >>> [[2, 10], [3, 15], [7, 12], [12, 0], [15, 10], [20, 8], [24, 0]]


      Here is the time taken for this output -



      %timeit output.get_skyline([[2, 9, 10], [3, 7, 15], [5, 12, 12], [15, 20, 10], [19, 24, 8]])
      >>> 27.6 µs ± 3.65 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)


      So, I would like to know whether I could make this program more efficient and/or shorter. Any alternatives are welcome.







      python performance python-3.x programming-challenge divide-and-conquer






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited May 28 at 18:44







      Justin

















      asked May 28 at 6:15









      JustinJustin

      1,170327




      1,170327




















          2 Answers
          2






          active

          oldest

          votes


















          5












          $begingroup$

          Nice approach.



          It's quite a hard problem to analyse for running time, because the output size is variable. merge has running time linear in the total size of its inputs, which are the output sizes of the subproblems. What we can say is that the base case produces two points for one input point, and merge doesn't create new points, so the number of points merged is at most twice the number of input points. Therefore the recurrence is $T(n) = 2T(n/2) + O(n)$ which is the standard recurrence giving $O(n lg n)$.



          We can also show that this can't be beaten by a reduction from sorting. Given a set $x_i$ to sort, we find the maximum $m$ and construct buildings $(0, m + 1 - x_i, x_i)$. The output will give the $x_i$ sorted in descending order.



          So your approach is asymptotically optimal, and moreover elegant. The merge doesn't do anything fancy, so it should have a low constant hidden by the big-O notation.




          I think that some of the double-indexed array accesses could benefit from introducing names. In particular, I would find




           return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]



          much more readable as




           l, r, h = buildings[0]
          return [[l, h], [r, 0]]




          The three cases in merge can be simplified considerably by using <=. I find is_valid inelegant, so I would eliminate it by keeping track of the "current" height. Introducing names for left[i][0] and right[j][0] I refactored your code to




           def merge(self, left, right):
          h1, h2, hcurrent = 0, 0, 0
          i, j = 0, 0
          result = []

          while i < len(left) and j < len(right):
          x0 = left[i][0]
          x1 = right[j][0]
          if x0 <= x1:
          h1 = left[i][1]
          i += 1
          if x1 <= x0:
          h2 = right[j][1]
          j += 1
          if max(h1, h2) != hcurrent:
          hcurrent = max(h1, h2)
          result.append([min(x0, x1), hcurrent])
          result.extend(right[j:])
          result.extend(left[i:])
          return result



          It could alternatively be refactored to use a sentinel as so:




           def merge(self, left, right):
          h1, h2 = 0, 0
          i, j = 0, 0
          result = [[0, 0]]

          while i < len(left) and j < len(right):
          x0 = left[i][0]
          x1 = right[j][0]
          if x0 <= x1:
          h1 = left[i][1]
          i += 1
          if x1 <= x0:
          h2 = right[j][1]
          j += 1
          if max(h1, h2) != result[-1][1]:
          result.append([min(x0, x1), max(h1, h2)])
          result.extend(right[j:])
          result.extend(left[i:])
          return result[1:]



          Either way, I think it would be good to add a comment explaining why




           result.extend(right[j:])
          result.extend(left[i:])



          doesn't need any extra checks to avoid producing two consecutive points at the same height.






          share|improve this answer











          $endgroup$




















            3












            $begingroup$

            In addition to the existing great answer of Peter Taylor I would like to add some thoughts on methods vs. free functions.



            Python is a multiparadigm language. It is concentrated on, but not only about object oriented programming.
            It is certainly possible to construct one large Solution class that contains all your methods, but it might be not the best Solution. (höhö)



            As a general guideline you can use classes to group data and functions acting on that data together.
            If you simply want to group functions together into one namespace, you should use namespaces i.e. modules in python.



            Let's get concrete and have a look at your is_valid method. It never uses self.
            If you want to keep the class structure, you should make this at least explicit and change it to:



            @staticmethod
            def is_valid(result, new_height):


            A staticmethod is basically a free function residing in the namespace of a class.



            But (opinionated) it might be even better in terms of reusability to completely "free" your function.
            If you come from a C++ background this makes it feel like a template function that you can apply on different inputs independent of the Solution class.



            If you "freed" is_valid you will realize, that merge does not depend on self either and if you "freed" merge you will finally realize that get_skyline is basically a recursive function that calls merge and can be a staticmethod or a free function itself.



            In the end you end up with a class of three staticmethods i.e. a namespace with three free functions. The canonical way of implementing this structure is to have those three function in their own module i.e. their own file.



            Practically speaking, just delete the class and references to self, dedent the methods and call your file Solutions.py. Then you will be able to call



            import Solutions
            Solutions.get_skyline(my_cool_skyline_test_data)


            which feels very similar in terms of syntax as your class approach but decoupled the functions from each other.



            If you think that a function like is_valid is an implementation detail of your module, you can prepend an underscore which makes it private by convention. This would allow to switch between an _is_valid function and the manual "current" height tracking suggested in the other answer.






            share|improve this answer









            $endgroup$








            • 1




              $begingroup$
              I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
              $endgroup$
              – Peter Taylor
              May 29 at 7:22











            Your Answer






            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            autoActivateHeartbeat: false,
            convertImagesToLinks: false,
            noModals: true,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            imageUploader:
            brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
            contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
            allowUrls: true
            ,
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );













            draft saved

            draft discarded


















            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f221178%2fpython-program-to-solve-the-skyline-problem%23new-answer', 'question_page');

            );

            Post as a guest















            Required, but never shown

























            2 Answers
            2






            active

            oldest

            votes








            2 Answers
            2






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes









            5












            $begingroup$

            Nice approach.



            It's quite a hard problem to analyse for running time, because the output size is variable. merge has running time linear in the total size of its inputs, which are the output sizes of the subproblems. What we can say is that the base case produces two points for one input point, and merge doesn't create new points, so the number of points merged is at most twice the number of input points. Therefore the recurrence is $T(n) = 2T(n/2) + O(n)$ which is the standard recurrence giving $O(n lg n)$.



            We can also show that this can't be beaten by a reduction from sorting. Given a set $x_i$ to sort, we find the maximum $m$ and construct buildings $(0, m + 1 - x_i, x_i)$. The output will give the $x_i$ sorted in descending order.



            So your approach is asymptotically optimal, and moreover elegant. The merge doesn't do anything fancy, so it should have a low constant hidden by the big-O notation.




            I think that some of the double-indexed array accesses could benefit from introducing names. In particular, I would find




             return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]



            much more readable as




             l, r, h = buildings[0]
            return [[l, h], [r, 0]]




            The three cases in merge can be simplified considerably by using <=. I find is_valid inelegant, so I would eliminate it by keeping track of the "current" height. Introducing names for left[i][0] and right[j][0] I refactored your code to




             def merge(self, left, right):
            h1, h2, hcurrent = 0, 0, 0
            i, j = 0, 0
            result = []

            while i < len(left) and j < len(right):
            x0 = left[i][0]
            x1 = right[j][0]
            if x0 <= x1:
            h1 = left[i][1]
            i += 1
            if x1 <= x0:
            h2 = right[j][1]
            j += 1
            if max(h1, h2) != hcurrent:
            hcurrent = max(h1, h2)
            result.append([min(x0, x1), hcurrent])
            result.extend(right[j:])
            result.extend(left[i:])
            return result



            It could alternatively be refactored to use a sentinel as so:




             def merge(self, left, right):
            h1, h2 = 0, 0
            i, j = 0, 0
            result = [[0, 0]]

            while i < len(left) and j < len(right):
            x0 = left[i][0]
            x1 = right[j][0]
            if x0 <= x1:
            h1 = left[i][1]
            i += 1
            if x1 <= x0:
            h2 = right[j][1]
            j += 1
            if max(h1, h2) != result[-1][1]:
            result.append([min(x0, x1), max(h1, h2)])
            result.extend(right[j:])
            result.extend(left[i:])
            return result[1:]



            Either way, I think it would be good to add a comment explaining why




             result.extend(right[j:])
            result.extend(left[i:])



            doesn't need any extra checks to avoid producing two consecutive points at the same height.






            share|improve this answer











            $endgroup$

















              5












              $begingroup$

              Nice approach.



              It's quite a hard problem to analyse for running time, because the output size is variable. merge has running time linear in the total size of its inputs, which are the output sizes of the subproblems. What we can say is that the base case produces two points for one input point, and merge doesn't create new points, so the number of points merged is at most twice the number of input points. Therefore the recurrence is $T(n) = 2T(n/2) + O(n)$ which is the standard recurrence giving $O(n lg n)$.



              We can also show that this can't be beaten by a reduction from sorting. Given a set $x_i$ to sort, we find the maximum $m$ and construct buildings $(0, m + 1 - x_i, x_i)$. The output will give the $x_i$ sorted in descending order.



              So your approach is asymptotically optimal, and moreover elegant. The merge doesn't do anything fancy, so it should have a low constant hidden by the big-O notation.




              I think that some of the double-indexed array accesses could benefit from introducing names. In particular, I would find




               return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]



              much more readable as




               l, r, h = buildings[0]
              return [[l, h], [r, 0]]




              The three cases in merge can be simplified considerably by using <=. I find is_valid inelegant, so I would eliminate it by keeping track of the "current" height. Introducing names for left[i][0] and right[j][0] I refactored your code to




               def merge(self, left, right):
              h1, h2, hcurrent = 0, 0, 0
              i, j = 0, 0
              result = []

              while i < len(left) and j < len(right):
              x0 = left[i][0]
              x1 = right[j][0]
              if x0 <= x1:
              h1 = left[i][1]
              i += 1
              if x1 <= x0:
              h2 = right[j][1]
              j += 1
              if max(h1, h2) != hcurrent:
              hcurrent = max(h1, h2)
              result.append([min(x0, x1), hcurrent])
              result.extend(right[j:])
              result.extend(left[i:])
              return result



              It could alternatively be refactored to use a sentinel as so:




               def merge(self, left, right):
              h1, h2 = 0, 0
              i, j = 0, 0
              result = [[0, 0]]

              while i < len(left) and j < len(right):
              x0 = left[i][0]
              x1 = right[j][0]
              if x0 <= x1:
              h1 = left[i][1]
              i += 1
              if x1 <= x0:
              h2 = right[j][1]
              j += 1
              if max(h1, h2) != result[-1][1]:
              result.append([min(x0, x1), max(h1, h2)])
              result.extend(right[j:])
              result.extend(left[i:])
              return result[1:]



              Either way, I think it would be good to add a comment explaining why




               result.extend(right[j:])
              result.extend(left[i:])



              doesn't need any extra checks to avoid producing two consecutive points at the same height.






              share|improve this answer











              $endgroup$















                5












                5








                5





                $begingroup$

                Nice approach.



                It's quite a hard problem to analyse for running time, because the output size is variable. merge has running time linear in the total size of its inputs, which are the output sizes of the subproblems. What we can say is that the base case produces two points for one input point, and merge doesn't create new points, so the number of points merged is at most twice the number of input points. Therefore the recurrence is $T(n) = 2T(n/2) + O(n)$ which is the standard recurrence giving $O(n lg n)$.



                We can also show that this can't be beaten by a reduction from sorting. Given a set $x_i$ to sort, we find the maximum $m$ and construct buildings $(0, m + 1 - x_i, x_i)$. The output will give the $x_i$ sorted in descending order.



                So your approach is asymptotically optimal, and moreover elegant. The merge doesn't do anything fancy, so it should have a low constant hidden by the big-O notation.




                I think that some of the double-indexed array accesses could benefit from introducing names. In particular, I would find




                 return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]



                much more readable as




                 l, r, h = buildings[0]
                return [[l, h], [r, 0]]




                The three cases in merge can be simplified considerably by using <=. I find is_valid inelegant, so I would eliminate it by keeping track of the "current" height. Introducing names for left[i][0] and right[j][0] I refactored your code to




                 def merge(self, left, right):
                h1, h2, hcurrent = 0, 0, 0
                i, j = 0, 0
                result = []

                while i < len(left) and j < len(right):
                x0 = left[i][0]
                x1 = right[j][0]
                if x0 <= x1:
                h1 = left[i][1]
                i += 1
                if x1 <= x0:
                h2 = right[j][1]
                j += 1
                if max(h1, h2) != hcurrent:
                hcurrent = max(h1, h2)
                result.append([min(x0, x1), hcurrent])
                result.extend(right[j:])
                result.extend(left[i:])
                return result



                It could alternatively be refactored to use a sentinel as so:




                 def merge(self, left, right):
                h1, h2 = 0, 0
                i, j = 0, 0
                result = [[0, 0]]

                while i < len(left) and j < len(right):
                x0 = left[i][0]
                x1 = right[j][0]
                if x0 <= x1:
                h1 = left[i][1]
                i += 1
                if x1 <= x0:
                h2 = right[j][1]
                j += 1
                if max(h1, h2) != result[-1][1]:
                result.append([min(x0, x1), max(h1, h2)])
                result.extend(right[j:])
                result.extend(left[i:])
                return result[1:]



                Either way, I think it would be good to add a comment explaining why




                 result.extend(right[j:])
                result.extend(left[i:])



                doesn't need any extra checks to avoid producing two consecutive points at the same height.






                share|improve this answer











                $endgroup$



                Nice approach.



                It's quite a hard problem to analyse for running time, because the output size is variable. merge has running time linear in the total size of its inputs, which are the output sizes of the subproblems. What we can say is that the base case produces two points for one input point, and merge doesn't create new points, so the number of points merged is at most twice the number of input points. Therefore the recurrence is $T(n) = 2T(n/2) + O(n)$ which is the standard recurrence giving $O(n lg n)$.



                We can also show that this can't be beaten by a reduction from sorting. Given a set $x_i$ to sort, we find the maximum $m$ and construct buildings $(0, m + 1 - x_i, x_i)$. The output will give the $x_i$ sorted in descending order.



                So your approach is asymptotically optimal, and moreover elegant. The merge doesn't do anything fancy, so it should have a low constant hidden by the big-O notation.




                I think that some of the double-indexed array accesses could benefit from introducing names. In particular, I would find




                 return [[buildings[0][0], buildings[0][2]], [buildings[0][1], 0]]



                much more readable as




                 l, r, h = buildings[0]
                return [[l, h], [r, 0]]




                The three cases in merge can be simplified considerably by using <=. I find is_valid inelegant, so I would eliminate it by keeping track of the "current" height. Introducing names for left[i][0] and right[j][0] I refactored your code to




                 def merge(self, left, right):
                h1, h2, hcurrent = 0, 0, 0
                i, j = 0, 0
                result = []

                while i < len(left) and j < len(right):
                x0 = left[i][0]
                x1 = right[j][0]
                if x0 <= x1:
                h1 = left[i][1]
                i += 1
                if x1 <= x0:
                h2 = right[j][1]
                j += 1
                if max(h1, h2) != hcurrent:
                hcurrent = max(h1, h2)
                result.append([min(x0, x1), hcurrent])
                result.extend(right[j:])
                result.extend(left[i:])
                return result



                It could alternatively be refactored to use a sentinel as so:




                 def merge(self, left, right):
                h1, h2 = 0, 0
                i, j = 0, 0
                result = [[0, 0]]

                while i < len(left) and j < len(right):
                x0 = left[i][0]
                x1 = right[j][0]
                if x0 <= x1:
                h1 = left[i][1]
                i += 1
                if x1 <= x0:
                h2 = right[j][1]
                j += 1
                if max(h1, h2) != result[-1][1]:
                result.append([min(x0, x1), max(h1, h2)])
                result.extend(right[j:])
                result.extend(left[i:])
                return result[1:]



                Either way, I think it would be good to add a comment explaining why




                 result.extend(right[j:])
                result.extend(left[i:])



                doesn't need any extra checks to avoid producing two consecutive points at the same height.







                share|improve this answer














                share|improve this answer



                share|improve this answer








                edited May 29 at 7:18

























                answered May 28 at 7:52









                Peter TaylorPeter Taylor

                19.6k3266




                19.6k3266























                    3












                    $begingroup$

                    In addition to the existing great answer of Peter Taylor I would like to add some thoughts on methods vs. free functions.



                    Python is a multiparadigm language. It is concentrated on, but not only about object oriented programming.
                    It is certainly possible to construct one large Solution class that contains all your methods, but it might be not the best Solution. (höhö)



                    As a general guideline you can use classes to group data and functions acting on that data together.
                    If you simply want to group functions together into one namespace, you should use namespaces i.e. modules in python.



                    Let's get concrete and have a look at your is_valid method. It never uses self.
                    If you want to keep the class structure, you should make this at least explicit and change it to:



                    @staticmethod
                    def is_valid(result, new_height):


                    A staticmethod is basically a free function residing in the namespace of a class.



                    But (opinionated) it might be even better in terms of reusability to completely "free" your function.
                    If you come from a C++ background this makes it feel like a template function that you can apply on different inputs independent of the Solution class.



                    If you "freed" is_valid you will realize, that merge does not depend on self either and if you "freed" merge you will finally realize that get_skyline is basically a recursive function that calls merge and can be a staticmethod or a free function itself.



                    In the end you end up with a class of three staticmethods i.e. a namespace with three free functions. The canonical way of implementing this structure is to have those three function in their own module i.e. their own file.



                    Practically speaking, just delete the class and references to self, dedent the methods and call your file Solutions.py. Then you will be able to call



                    import Solutions
                    Solutions.get_skyline(my_cool_skyline_test_data)


                    which feels very similar in terms of syntax as your class approach but decoupled the functions from each other.



                    If you think that a function like is_valid is an implementation detail of your module, you can prepend an underscore which makes it private by convention. This would allow to switch between an _is_valid function and the manual "current" height tracking suggested in the other answer.






                    share|improve this answer









                    $endgroup$








                    • 1




                      $begingroup$
                      I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                      $endgroup$
                      – Peter Taylor
                      May 29 at 7:22















                    3












                    $begingroup$

                    In addition to the existing great answer of Peter Taylor I would like to add some thoughts on methods vs. free functions.



                    Python is a multiparadigm language. It is concentrated on, but not only about object oriented programming.
                    It is certainly possible to construct one large Solution class that contains all your methods, but it might be not the best Solution. (höhö)



                    As a general guideline you can use classes to group data and functions acting on that data together.
                    If you simply want to group functions together into one namespace, you should use namespaces i.e. modules in python.



                    Let's get concrete and have a look at your is_valid method. It never uses self.
                    If you want to keep the class structure, you should make this at least explicit and change it to:



                    @staticmethod
                    def is_valid(result, new_height):


                    A staticmethod is basically a free function residing in the namespace of a class.



                    But (opinionated) it might be even better in terms of reusability to completely "free" your function.
                    If you come from a C++ background this makes it feel like a template function that you can apply on different inputs independent of the Solution class.



                    If you "freed" is_valid you will realize, that merge does not depend on self either and if you "freed" merge you will finally realize that get_skyline is basically a recursive function that calls merge and can be a staticmethod or a free function itself.



                    In the end you end up with a class of three staticmethods i.e. a namespace with three free functions. The canonical way of implementing this structure is to have those three function in their own module i.e. their own file.



                    Practically speaking, just delete the class and references to self, dedent the methods and call your file Solutions.py. Then you will be able to call



                    import Solutions
                    Solutions.get_skyline(my_cool_skyline_test_data)


                    which feels very similar in terms of syntax as your class approach but decoupled the functions from each other.



                    If you think that a function like is_valid is an implementation detail of your module, you can prepend an underscore which makes it private by convention. This would allow to switch between an _is_valid function and the manual "current" height tracking suggested in the other answer.






                    share|improve this answer









                    $endgroup$








                    • 1




                      $begingroup$
                      I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                      $endgroup$
                      – Peter Taylor
                      May 29 at 7:22













                    3












                    3








                    3





                    $begingroup$

                    In addition to the existing great answer of Peter Taylor I would like to add some thoughts on methods vs. free functions.



                    Python is a multiparadigm language. It is concentrated on, but not only about object oriented programming.
                    It is certainly possible to construct one large Solution class that contains all your methods, but it might be not the best Solution. (höhö)



                    As a general guideline you can use classes to group data and functions acting on that data together.
                    If you simply want to group functions together into one namespace, you should use namespaces i.e. modules in python.



                    Let's get concrete and have a look at your is_valid method. It never uses self.
                    If you want to keep the class structure, you should make this at least explicit and change it to:



                    @staticmethod
                    def is_valid(result, new_height):


                    A staticmethod is basically a free function residing in the namespace of a class.



                    But (opinionated) it might be even better in terms of reusability to completely "free" your function.
                    If you come from a C++ background this makes it feel like a template function that you can apply on different inputs independent of the Solution class.



                    If you "freed" is_valid you will realize, that merge does not depend on self either and if you "freed" merge you will finally realize that get_skyline is basically a recursive function that calls merge and can be a staticmethod or a free function itself.



                    In the end you end up with a class of three staticmethods i.e. a namespace with three free functions. The canonical way of implementing this structure is to have those three function in their own module i.e. their own file.



                    Practically speaking, just delete the class and references to self, dedent the methods and call your file Solutions.py. Then you will be able to call



                    import Solutions
                    Solutions.get_skyline(my_cool_skyline_test_data)


                    which feels very similar in terms of syntax as your class approach but decoupled the functions from each other.



                    If you think that a function like is_valid is an implementation detail of your module, you can prepend an underscore which makes it private by convention. This would allow to switch between an _is_valid function and the manual "current" height tracking suggested in the other answer.






                    share|improve this answer









                    $endgroup$



                    In addition to the existing great answer of Peter Taylor I would like to add some thoughts on methods vs. free functions.



                    Python is a multiparadigm language. It is concentrated on, but not only about object oriented programming.
                    It is certainly possible to construct one large Solution class that contains all your methods, but it might be not the best Solution. (höhö)



                    As a general guideline you can use classes to group data and functions acting on that data together.
                    If you simply want to group functions together into one namespace, you should use namespaces i.e. modules in python.



                    Let's get concrete and have a look at your is_valid method. It never uses self.
                    If you want to keep the class structure, you should make this at least explicit and change it to:



                    @staticmethod
                    def is_valid(result, new_height):


                    A staticmethod is basically a free function residing in the namespace of a class.



                    But (opinionated) it might be even better in terms of reusability to completely "free" your function.
                    If you come from a C++ background this makes it feel like a template function that you can apply on different inputs independent of the Solution class.



                    If you "freed" is_valid you will realize, that merge does not depend on self either and if you "freed" merge you will finally realize that get_skyline is basically a recursive function that calls merge and can be a staticmethod or a free function itself.



                    In the end you end up with a class of three staticmethods i.e. a namespace with three free functions. The canonical way of implementing this structure is to have those three function in their own module i.e. their own file.



                    Practically speaking, just delete the class and references to self, dedent the methods and call your file Solutions.py. Then you will be able to call



                    import Solutions
                    Solutions.get_skyline(my_cool_skyline_test_data)


                    which feels very similar in terms of syntax as your class approach but decoupled the functions from each other.



                    If you think that a function like is_valid is an implementation detail of your module, you can prepend an underscore which makes it private by convention. This would allow to switch between an _is_valid function and the manual "current" height tracking suggested in the other answer.







                    share|improve this answer












                    share|improve this answer



                    share|improve this answer










                    answered May 28 at 21:21









                    mcocdawcmcocdawc

                    329110




                    329110







                    • 1




                      $begingroup$
                      I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                      $endgroup$
                      – Peter Taylor
                      May 29 at 7:22












                    • 1




                      $begingroup$
                      I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                      $endgroup$
                      – Peter Taylor
                      May 29 at 7:22







                    1




                    1




                    $begingroup$
                    I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                    $endgroup$
                    – Peter Taylor
                    May 29 at 7:22




                    $begingroup$
                    I think the use of a class is forced by the testing framework OP is working in, but that shouldn't detract from this answer, which gives a great exposition.
                    $endgroup$
                    – Peter Taylor
                    May 29 at 7:22

















                    draft saved

                    draft discarded
















































                    Thanks for contributing an answer to Code Review Stack Exchange!


                    • Please be sure to answer the question. Provide details and share your research!

                    But avoid


                    • Asking for help, clarification, or responding to other answers.

                    • Making statements based on opinion; back them up with references or personal experience.

                    Use MathJax to format equations. MathJax reference.


                    To learn more, see our tips on writing great answers.




                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function ()
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f221178%2fpython-program-to-solve-the-skyline-problem%23new-answer', 'question_page');

                    );

                    Post as a guest















                    Required, but never shown





















































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown

































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown







                    Popular posts from this blog

                    Grendel Contents Story Scholarship Depictions Notes References Navigation menu10.1093/notesj/gjn112Berserkeree

                    Area configuration aggregation error after install Porto themeMagento 2.1 CE Installed but front/backend not loading/workingCSS not loading on page within Magento 2 pageCannot install module in Magento 2no commands defined in the “setup” namespace. in Magento2Magento 2: Static files are present but shows 404Why do i have to always run the commands to clean cache in Magento 2.1.8?Failure reason: 'Unable to unserialize value.'Error 500 after magento migrationIn production mode the site does not loadMagento 2 : Error 500 after installing

                    Middle Expansion Olielle Resaix Definition: Uttering songs of triumph shouting with joy triumphant exulting Sejunction Journal 붙다 달 고급 품목 외출 The stretch trades the screeching tin. Definition: The act of speaking with a drawl a drawl Cough Sand Definition: An uproar a quarrel a noisy outbreak Shake Iron Publicize Horse House Baby 사과 Resaix Flaggy Jelly Temporary Unequaled Puppet A drop in the bucket Shrew 성격 회원 성질 미팅 The burn frames the tacky quality. Materialistic The smoke reduces the way. Yammoe Nondescript Cheek 얼굴 배 약하다 날리다 타다 The illegal country shows the iron. Help Rule Drearien Smoke Teaching Meaty Wasp Abraham Lincoln Jaws 진심 수리하다 Size Cork Idea Convert Think Lark John Lennon 거울 청소 군 추천하다 아이스크림