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;
$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).
The geometric information of each building is represented by a triplet
of integers[Li, Ri, Hi], whereLiandRiare thex
coordinates of the left and right edge of theith building,
respectively, andHiis its height. It is guaranteed that0 ≤ Li,,
Ri ≤ INT_MAX0 < Hi ≤ INT_MAX, andRi - Li > 0. You may assume
all buildings are perfect rectangles grounded on an absolutely flat
surface at height0.
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
positionLi.
The output list must be sorted by the
xposition.
There must be no consecutive horizontal lines of equal height in the
output skyline. For instance,[...[2 3], [4 5], [7 5], [11 5], [12is not acceptable; the three lines of height 5 should be
7]...]
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
$endgroup$
add a comment |
$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).
The geometric information of each building is represented by a triplet
of integers[Li, Ri, Hi], whereLiandRiare thex
coordinates of the left and right edge of theith building,
respectively, andHiis its height. It is guaranteed that0 ≤ Li,,
Ri ≤ INT_MAX0 < Hi ≤ INT_MAX, andRi - Li > 0. You may assume
all buildings are perfect rectangles grounded on an absolutely flat
surface at height0.
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
positionLi.
The output list must be sorted by the
xposition.
There must be no consecutive horizontal lines of equal height in the
output skyline. For instance,[...[2 3], [4 5], [7 5], [11 5], [12is not acceptable; the three lines of height 5 should be
7]...]
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
$endgroup$
add a comment |
$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).
The geometric information of each building is represented by a triplet
of integers[Li, Ri, Hi], whereLiandRiare thex
coordinates of the left and right edge of theith building,
respectively, andHiis its height. It is guaranteed that0 ≤ Li,,
Ri ≤ INT_MAX0 < Hi ≤ INT_MAX, andRi - Li > 0. You may assume
all buildings are perfect rectangles grounded on an absolutely flat
surface at height0.
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
positionLi.
The output list must be sorted by the
xposition.
There must be no consecutive horizontal lines of equal height in the
output skyline. For instance,[...[2 3], [4 5], [7 5], [11 5], [12is not acceptable; the three lines of height 5 should be
7]...]
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
$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).
The geometric information of each building is represented by a triplet
of integers[Li, Ri, Hi], whereLiandRiare thex
coordinates of the left and right edge of theith building,
respectively, andHiis its height. It is guaranteed that0 ≤ Li,,
Ri ≤ INT_MAX0 < Hi ≤ INT_MAX, andRi - Li > 0. You may assume
all buildings are perfect rectangles grounded on an absolutely flat
surface at height0.
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
positionLi.
The output list must be sorted by the
xposition.
There must be no consecutive horizontal lines of equal height in the
output skyline. For instance,[...[2 3], [4 5], [7 5], [11 5], [12is not acceptable; the three lines of height 5 should be
7]...]
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
python performance python-3.x programming-challenge divide-and-conquer
edited May 28 at 18:44
Justin
asked May 28 at 6:15
JustinJustin
1,170327
1,170327
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
$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.
$endgroup$
add a comment |
$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.
$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
add a comment |
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
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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.
$endgroup$
add a comment |
$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.
$endgroup$
add a comment |
$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.
$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.
edited May 29 at 7:18
answered May 28 at 7:52
Peter TaylorPeter Taylor
19.6k3266
19.6k3266
add a comment |
add a comment |
$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.
$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
add a comment |
$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.
$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
add a comment |
$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.
$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.
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
add a comment |
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
add a comment |
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.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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

