Hot answers tagged

43

I'm going to reuse some parts of the answer I recently posted here on Code Review. Losing your Loops (Most) loops are damn slow in Python. Especially multiple nested loops. NumPy can help to vectorize your code, i.e. in this case that more of the looping is done in the C backend instead of in the Python interpreter. I would highly recommend to ...


18

This will cover performance, as well as Python style. Save constants in one place You currently have the magic numbers 2000 and 3000, the resolution of your image. Save these to variables perhaps named X, Y or W, H. Mention your requirements You don't just rely on Python 3 and Jupyter - you rely on numpy and pillow. These should go in a requirements.txt ...


13

This is a tip I make a lot, but if you have a collection that's simply tracking "membership", and you don't care about order, you should consider using a Set over a List. I think this is the case for cell.linked_cells. The only thing you ever do with cell.linked_cells is do in membership tests, and add and remove from it. Make the following changes: ...


13

Here are some suggestions for improving the code. Use all required #includes The code uses vector but doesn't include the corresponding header. The code should have #include <vector> Use <cmath> instead of <math.h> The difference between the two forms is that the former defines things within the std:: namespace versus into the global ...


12

Don't use vectorized numpy, use numba jit instead Using numpy to calculate the Mandelbrot set is not really a good fit because the same data will be stored and loaded from and to memory repeatedly, thrashing the cache. A better option would be to use a jit compiler to accelerate the critical code path, for example numba jit. In this case, 4 characters can ...


11

Mandelbrot-specific optimisations These can be combined with the Python-specific optimisations from the other answers. Avoid the redundant square root if (z.real**2+z.imag**2)**0.5 > 2: is equivalent to if z.real ** 2 + z.imag ** 2 > 4: (simply square both sides of the original comparison to get the optimised comparison) Avoid squaring unless ...


8

In terms of making more robust, a fairly obvious improvement would be to ensure that argv[1] exists before using it (that's what argc is for). If there's no argument, or the file can't be read, emit a useful error message to std::cerr and return EXIT_FAILURE - negative return values from main() can vary by platform. Why do we declare CannyThreshold with ...


7

Wow! I’ve never reviewed an image before. Neat. First, I’d flip your left square to align the corners of the right-most triangles in the left square with the horizontal line in the right square. This gives a visual indication that those dimensions (a and b) in both squares are the same. With the original image, your eye has to draw the line all the way ...


7

In addition to what was mentioned in the review by @nivag. With the exception of the use of std::vector the code looks a lot more like C than any version of C++. Proper Include Headers The code under review does not require stdio.h. Please note that in C++ C programming include files can be included by inserting 'c' before the header name and removing ...


6

Code: Don't use using namespace std;. It looks like you're including more dependencies in the header than necessary (cmath, fstream, cctype). These are only used in the .cpp file, so they should be included there. Use enum class for type safety instead of a plain enum. We don't need to call imageData.clear() in the destructor, or even define a destructor. (...


6

I'm not a python expert. I am pretty good with Mandlebrot generation (I've spent a lot of time on my custom Julia Set generator.) So I'll say this: optimize the heck out of stuff that will be running many iterations. Forget about clean-code or nice OOP principles. For lots-of-iterations stuff like this, you want as nitty gritty as possible. So let's ...


6

I would like to share some observations about your main concerns given at the end of the question. Let's start from the back: 5. extend_col/reduce_row From what I can see, the "trick" here is to bring the points into a homogenous coordinate system and back. Therefore, I would propose to change the name of both functions to to_homogeneous and ...


6

Type hints row_index, column_index, rows, columns I can guess that these are all int based on the docs. But adding :int (etc) will help, here. Variable reuse north = self.row - 1, self.column if north[0] < 0: north = 0 neighbors.append(0) if north: neighbors.append(grid[north[0]][north[1]]) This is confusing. north ...


6

var zipFile = ZipFile.OpenRead(ChapterLocation); ... zipFile.Dispose(); One word: using. using (var stream = z.Open()) { using (var mstream = new MemoryStream()) { stream.CopyTo(mstream); mstream.Position = 0; var bitmap = new BitmapImage(); bitmap.BeginInit(); ...


6

Throwaway memory I haven't done any analysis on the code but based on my experience the worst part is the allocation of throwaway memory inside the innermost loop. Instead of allocating a 3x3 array 9 million times and throwing it to the garbage colletor immediately, allocate it once at the start of the method and reuse it in the loop. int[][] kernel = new ...


6

There are a number of things I would change here: gradient_mag is unused in HoughTransformCircles and should be removed. Mat will presumably normally be a relatively large array so relatively expensive to copy, additionally you don't modify image or gradient_dir in the code. These can both be passed in as const Mat &. You should make variables that ...


5

Indentation First of all, format your code wisely. Do not make your main program body shifted by five levels of indentation. It makes it awfully hard to read (and review, mind you). Fail early. If a condition makes the further code execution impossible - just make it throw an error and then keep writing the following code on the same level, like ...


5

public static Jpeg FromBitmapFrame(BitmapFrame bitmap, long quality) { Jpeg jpeg; using (var stream = new MemoryStream()) { JpegBitmapEncoder encoder = new JpegBitmapEncoder(); encoder.QualityLevel = 90; encoder.Frames.Add(bitmap); encoder.Save(stream); jpeg = new Jpeg(stream.ToArray(), (uint)bitmap.Width, (uint)bitmap.Height, (uint)...


5

Reversing the condition of if (originalImage != null) and returning early will remove one level of indentation. The less indentation some code shows the easier to read it will become. catch to just rethrow, althought you did it in the correct way, doesn't buy you anything. Just remove the try..catch..finally and enclose the usage of graphics with an usage ...


4

The two things that currently bother me most about this code are Naming and Abstraction. Don't get me wrong, this is something pretty cool and it's really well-crafted for the most part. But I'm still waiting for this to go the last mile: There is 12 local variables or functions in your main. I reckon you can easily reduce that to half as many by adding ...


4

For SIMD optimization, I tried doing it with SSE2, as the compilation flags imply SSSE3 is disabled, and SSE3 is not useful here. It's mostly a transliteration of the scalar code, but there are a few points of interest: Except the squaring of gx and gy which is done in 32bit, and taking the square root which is done in floating point, most arithmetic is ...


4

Henrik covered a number of points which I would have raised, so I won't repeat those. namespace TIFF { public class Jpeg seems inconsistent to me. The convention in .Net is to camel-case acronyms and initialisms, so the class name is as expected and the namespace is not. public static class JpegTiff { public static byte[] Create(List&...


4

I spent sometime with the code you've currently posted @wizofe, I'm not sure if it'll ever be the same again... Side note; I heard it rumored that the character limits are just a bit more relaxed than on other sub-stacks... or in other-words this may get a bit verbose, or in other-other-words it could be another one of those posts so a snack and drink is ...


4

This is clean code for a hobbyist. ML code in particular can end up being a procedural mess, just carrying out one imported function after another, but this is quite good. It's great that you have docstrings with explanations of the arguments, but the explanations of the functions/methods themselves are a little sparse. The odd comment would help a lot too. ...


4

You have a god-class Mozaika, you should define image mutations on another class Image. You have three mutating containers that hold the information you need. This is really really really bad. If I were an interviewer the second I see that I'd know I wouldn't want you. This is because it makes your code hard to read, and really fragile. Below is what, a ...


4

As you specifically ask for a different framework: you reinvented the wheel here. The base libraries of java already contain everything necessary to do this operation in a few lines of code while utilizing highly optimized vendor code. For a blur operation, create a kernel of 1/9 in 3x3, e.g. float oneNinth = 1f / 9f; Kernel kernel = new Kernel(...


4

I have a few different comments. Firstly, for 2D arrays like in your example, you can simply do conv2(binaryImage,structuringElement,'same'), it's equivalent to imdilate (and about 4 times faster). That might not be acceptable for your case if you are supposed to write the code yourself though. Next, and importantly, your code has a logic error. If your ...


3

Both Prewitt and Sobel filters are "separable". To calculate the gradients (gx and gy), the original code effectively iterates over and sums a 2d kernel (i.e. convolution). For the Prewitt and Sobel filters, each gradient kernel can be split into two 1D kernels, and we can do the calculation of each gradient in two passes. For a k sized filter, this reduces ...


3

The way I see it your requirements can be summarized as: Have a central repository of images. Load Images from a some data source. Save Persist Images. Give a cell its appropriate image based on an identifier. The main issue I see is that the cells are tightly coupled to the ImageCache object, it might be better to have some sort of ViewModel or controller ...


3

At least to me, the naming of swapChannel is confusing (and I go "how can the parameter be const if the method does a swap?"). The method is not swapping, but it is simply "reversing" the channels from arr to dst. I would consider a more informative name for the method (for a swap, I'd expect two parameters by-reference and then doing std::swaps inside). If ...


Only top voted, non community-wiki answers of a minimum length are eligible