18
Search Space
Your search space is too large. Since 0 < a < b < c and a + b + c = 1000, you can put hard limits on both a and c. The maximum a can be is 332, since 333 + 334 + 335 > 1000. Similarly, the minimum value c can be is 335, since 332 + 333 + 334 < 1000. As well, the maximum value c can have is 997 (1 + 2 + 997 = 1000).
Based on ...
11
Note that your code actually gives the wrong answer, it returns a > b.
Since a and b are the two smaller variables, you should actually start looping over them in order, so it looks more like:
for a in range(1, 1000): # Don't know why you assumed a > 1 here
for b in range(a + 1, 1000):
c = 1000 - a - b
Also, it doesn't matter for this ...
8
Instead of returning a custom string, just return the values and leave the printing to the caller. This way it is at least feasible for this function to be used elsewhere.
def get_triplet():
...
return a, b, c
if __name__ == '__main__':
a, b, c = get_triplet()
print(f"a = {a}, b = {b}, c = {c}, product = {a*b*c}")
Here I also used an f-...
8
If you want a solution that doesn't involve any coding, you can use the fact that Pythagorean triples \$a < b < c\$ are of the form
\$a = 2pqr\$
\$b = p(q^2 - r^2)\$
\$c = p(q^2 + r^2)\$
or the same equations with \$a\$ and \$b\$ switched.
Here \$p, q, r\$ are positive integers, which are uniquely determined by the condition
\$p = \gcd(a, b, c)\$....
7
O(n)
You can transform the (row,column) coordinate of a bishop to a (row+column, row-column) coordinate. The row+column coordinate tells you which upper-left to lower right diagonal the bishop is on. The row-column coordinate tells you which upper-right to lower-left diagonal the bishop is on.
row + col row - col
7 8 9 10 11 ...
7
DRY
This repetitive code is an anti-pattern:
blueTotal = 0
greenTotal = 0
redTotal = 0
yellowTotal = 0
Please define
houses = 'blue green red yellow'.split()
and then you can use array access to perform "the same action" across all houses:
for house in houses:
total[house] = 0
One could also assign total = collections.defaultdict(int), but that ...
6
Integer Division
In Python, 10 / 2 is equal to 5.0, not 5. Python has the integer division operator (//) which produces an integral value after division, instead of a floating point value. To prevent storing both int and float keys in the count dictionary, you should use:
count[n] = collatz_count(n // 2) + 1
Cache
Your count cache works nicely. ...
5
simplify a and fix search space
Given the nested for loop, your variable a can be decremented simpler:
Moreover, your search space is far too large. Other authors have addressed his in more detail, but if you want to check every possible triplet from (1-1000, then you need to change your second for loop to:
def get_triplet():
for c in range(2, 1000):
...
5
Here are some hints on how to decrease the size of your code and make it more Pythonic.
Stack Ifs
Anytime you see stacked ifs, that are all basically the same, you should consider using a dict. In the case here, the primary thing the ifs were doing was mapping a race place to points. So let's do that explicitly like:
pointvalues = {
0: 15,
...
5
Solution 1
You have two equations and three unknowns. Since three minus two is one, you should only require one search loop.
a + b + c = 1000
=> c = 1000 - a - b
a2 + b2 = c2
=> a2 + b2 = (1000 - a - b)2
=> a2 + b2 = 10002 - 2000a - 2000b + 2ab + a2 + b2
=> 0 = 1000000 - 2000a - 2000b + 2ab
=> 0 = 500000 - 1000a - 1000b + ab
=> 1000b - ab = 500000 - ...
4
Your game has the same kind of flaw like a lot of the other games that have recently been here on Code Review: an unnecessary recursion in the main game flow:
What do I mean by that? Let's look at your main function:
def main():
# ... all of the actual game here ...
if play_again():
main()
else:
print("Goodbye!")
...
4
If bishop X, Y & Z are all on a diagonal, X can capture Y, Y can capture both X & Z, and Z can capture Y. All 3 are unsafe. If the problem has no requirement to determine exactly which bishop captures which bishops -- just determine the number which could be captured -- then you don't need to move each bishop along the each of the four dx, dy in ...
4
As a good practice to promote code reuse, the function should return a numeric result. The print() should be done by the caller.
Constantly referring to is_previous_vowel is tedious. There are a couple of solutions you could use involving itertools. In particular, groupby() and zip_longest() could be useful.
Ultimately, the simplest solution is to use a ...
3
A few things no one has touched on yet...
points()
Your points values follow a pattern which can be easily modeled mathematically:
# Assign points from 1st = 15 through to 10-Last being 1
def points():
global pointvalue
if count == 0:
point_value = 15
places()
elif count < 3:
point_value = 14 - 2 * count
...
3
1) you should use the format function for Python when printing like so:
print("There are {} moves".format(len(captured)))
2) you should use if-elif whenever possible:
if (x, y) in seen:
break
elif (x, y) in remaining:
remaining.remove((x, y))
capture = f"{coordinates} takes {x, y}"
captured.append(capture)
break
3
I think this is mistake:
self.iterable = list(iterable)
It requires loading the entire iterable into memory. Being able to avoid this and work with large collections efficiently is one of the big benefits of iterators.
It won't work on non-ending iterators like itertools.count()
If instead, you make an iterator from the iterable, you can avoid both those ...
3
Here's one:
number_regex = re.compile(
r'^[-+]?(?:(?:(?:[1-9](?:_?\d)*|0+(_?0)*)|(?:0[bB](?:_?[01])+)'
r'|(?:0[oO](?:_?[0-7])+)|(?:0[xX](?:_?[0-9a-fA-F])+))'
r'|(?:(?:(?:\d(?:_?\d)*)?(?:\.(?:\d(?:_?\d)*))|(?:\d(?:_?\d)*)\.)'
r'|(?:(?:(?:\d(?:_?\d)*)|(?:(?:\d(?:_?\d)*)?(?:\.(?:\d(?:_?\d)*))'
r'|(?:\d(?:_?\d)*)\.))(?:[eE][-+]?(?:\d(?:_?\d)*...
3
Your expression looks just fine, maybe we would slightly modify that to:
^[+-]?((\d+(\.\d+)?)|(\.\d+))$
for failing these samples, 3., 4., for instance, just in case maybe such samples might be undesired. Other than that, you have some capturing groups that I'm guessing you'd like to keep those.
Test the capturing groups with re.finditer
import re
...
3
Congratulations to you for finding a major bottleneck yourself. There are a few more to get rid of.
Generally speaking it's quite a performance killer to convert data between Python and numpy repeatedly. And you do that a lot. You even do it to determine the number of iterations for some loops, e.g. in for j in range(np.array(alphas).shape[1]):. Since ...
3
You have lots of unneeded empty lines, they also hinder readability.
fn(arg=value) is abnormal. It's common to use fn(value). It's common only to use the former when the argument is a keyword or default argument.
You don't need to make a path another path.
desination_root = Path("R:\\Pictures")
destination = Path(desination_root).joinpath(folder.stem)
Path....
3
I would recommend just using the built in functionality of print to do the flushing. Though I am an advocate for breaking things out into classes and functions, you should use built in functionality when you can.
Making code complex for no reason results in two things:
1) More bugs.
2) More programmers who want to wring your neck down the road because ...
2
This is going to be relatively long, but I don't have a TL;DR section
if debug: print
Whenever you see code repeated like this, it's quite often you can refactor it into either a function, or there's a builtin to support it. Fortunately, the logging module makes this quite simple. You do lose a bit of speed over the if statement, but it's a much easier ...
2
fscript.close doesn't do anything. Perhaps you meant to call fscript.close(). In any case, the preferred way to open a file is to use a with block, so that Python always takes care of closing the file for you:
try:
with open("Script to use: ") as fscript:
script = fscript.read()
except IOError:
exitmsg("Unable to read file")
2
Whereas unlikely to have high impact, I have found a potential source of overfitting in your code:
# lets normalize the data
def normalize(input_data):
minimum = input_data.min(axis=0)
maximum = input_data.max(axis=0)
#normalized = (input_data - minimum) / ( maximum - minimum )
normalized = preprocessing.normalize(input_data, norm='l2')
...
2
Well, your regex solution is obviously badly broken - it will fail if the attributes are in a different order, if they are separated by newlines, if they are delimited by single quotes, etc etc. If you try to replace it with a more correct regex (it will never be 100% correct of course) then you are quite likely to lose some of this speed - perhaps ...
2
Suggested:
#!/usr/bin/python
import os
import requests
from bs4 import BeautifulSoup as bs
from pathlib import Path
from shutil import copyfileobj
MAX_PAGES = 1 # Max. number of pages is 41
SAVE_DIRECTORY = Path('fox_backgrounds')
BASE_URL = 'http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page'
RESOLUTIONS = {
'1280x800', '...
2
General suggestions:
black can automatically format your code to be more idiomatic.
isort can group and sort your imports automatically.
flake8 with a strict complexity limit will give you more hints to write idiomatic Python:
[flake8]
max-complexity = 4
ignore = W503,E203
That limit is not absolute by any means, but it's worth thinking hard whether you ...
2
Some suggestions:
number and n could be parameters to this script. That way the whole thing would be reusable.
You can use list comprehensions to partition your string:
>>> def partition(digits: str, length: int) -> List[str]:
... return [digits[index:index + length] for index in range(len(digits) - length + 1)]
...
>>> partition("...
2
I'm in a rush so this is half baked :(
It's best if you provide everything, currently your code isn't PEP 8 compliant due to you removing the docstrings. Some people may moan at you for this.
You should prefer guard statements over if else statements when possible. You can easily change if os.path.getsize(source_txt) to use a not which would reduce the ...
2
Extract day.title() to a separate method. In this case, if you wanted to change how you store the days of the weeks (e.g. Monday, Tuesday ... -> monday, tuesday...), you would have to change day.title() to day.lower() only in one place*.
Implement set_start_and_end() by calling set_start() and set_end(). Identically for getters. The rationale is similar to ...
Only top voted, non community-wiki answers of a minimum length are eligible
