Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. Don't use input as a variable name! input is used to ask users for a command line input. Using it as a variable name makes the function useless, but will also potentially create strange errors or warnings. (I called it vector in this review. You should pick a more descriptive name)

    The same goes with max, mean, length etc. Don't use function names as variable names.

  2. Your input is a vector, so no need to specify that you always want the first column. Use linear indexing instead of subscripts. It's much faster, and you won't mess up the dimensions (oops, it was a column vector, not a row vector).

  3. Don't use size(vector,1), and don't use length(vector) length(vector), use numel(vector). It's faster and more robust.

  4. You are pre-allocating output. Very good! That's a common bottleneck.

  5. output = vector*0; is clever. Prior to Matlab R2015b zeros(n,1) was actually far from the fastest way to create a vector of zeros! In fact, output(n,1) = 0 was up to 500 times faster! This is because zeros creates a matrix, then fills the elements with zeros, where as the latter just creates a matrix, where the values are zero by default.

  6. In your loop, don't use i as the iterator, it is used for the imaginary unit (sqrt(-1)). It's common to use for instance ii, jj etc.

  7. If your density function is only what you have posted there, then you can simply create an anonymous function inside the main function:

     density = @(vector) 1000*(1 - (vector+288.9414)./(508929.2.*(vector+68.12963)).*(vector-3.9863).^2);
    

    Call it like:

     dens = density(vector);
    
  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, don't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question this question on SO from three years ago.

  1. Don't use input as a variable name! input is used to ask users for a command line input. Using it as a variable name makes the function useless, but will also potentially create strange errors or warnings. (I called it vector in this review. You should pick a more descriptive name)

    The same goes with max, mean, length etc. Don't use function names as variable names.

  2. Your input is a vector, so no need to specify that you always want the first column. Use linear indexing instead of subscripts. It's much faster, and you won't mess up the dimensions (oops, it was a column vector, not a row vector).

  3. Don't use size(vector,1), and don't use length(vector), use numel(vector). It's faster and more robust.

  4. You are pre-allocating output. Very good! That's a common bottleneck.

  5. output = vector*0; is clever. Prior to Matlab R2015b zeros(n,1) was actually far from the fastest way to create a vector of zeros! In fact, output(n,1) = 0 was up to 500 times faster! This is because zeros creates a matrix, then fills the elements with zeros, where as the latter just creates a matrix, where the values are zero by default.

  6. In your loop, don't use i as the iterator, it is used for the imaginary unit (sqrt(-1)). It's common to use for instance ii, jj etc.

  7. If your density function is only what you have posted there, then you can simply create an anonymous function inside the main function:

     density = @(vector) 1000*(1 - (vector+288.9414)./(508929.2.*(vector+68.12963)).*(vector-3.9863).^2);
    

    Call it like:

     dens = density(vector);
    
  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, don't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question on SO from three years ago.

  1. Don't use input as a variable name! input is used to ask users for a command line input. Using it as a variable name makes the function useless, but will also potentially create strange errors or warnings. (I called it vector in this review. You should pick a more descriptive name)

    The same goes with max, mean, length etc. Don't use function names as variable names.

  2. Your input is a vector, so no need to specify that you always want the first column. Use linear indexing instead of subscripts. It's much faster, and you won't mess up the dimensions (oops, it was a column vector, not a row vector).

  3. Don't use size(vector,1), and don't use length(vector), use numel(vector). It's faster and more robust.

  4. You are pre-allocating output. Very good! That's a common bottleneck.

  5. output = vector*0; is clever. Prior to Matlab R2015b zeros(n,1) was actually far from the fastest way to create a vector of zeros! In fact, output(n,1) = 0 was up to 500 times faster! This is because zeros creates a matrix, then fills the elements with zeros, where as the latter just creates a matrix, where the values are zero by default.

  6. In your loop, don't use i as the iterator, it is used for the imaginary unit (sqrt(-1)). It's common to use for instance ii, jj etc.

  7. If your density function is only what you have posted there, then you can simply create an anonymous function inside the main function:

     density = @(vector) 1000*(1 - (vector+288.9414)./(508929.2.*(vector+68.12963)).*(vector-3.9863).^2);
    

    Call it like:

     dens = density(vector);
    
  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, don't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question on SO from three years ago.

added 1 character in body
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, _don't use tic/tocdon't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question on SO from three years ago.

  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, _don't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question on SO from three years ago.

  1. "You don't need to specify that 1 when indexing a vector, MATLAB knows". Many Matlab users use max(A), when they want to find the maximum value of all columns in a matrix. It's much simpler than max(A, [], 1). You might however come into trouble if A has varying number of rows (and matrices often do). Because the moment that matrix has only one row, max(A) will give the maximum value of the entire vector, instead of each (one row) column. This can give you a major headache!

  2. "Why not use length?" Well, in this case, length is much better than size, but numel should still be the preferred choice. length is better than size for two reasons. Using size limits the function to only work on column vectors only, and will fail on row vectors. Second, numel is much faster than length for large vectors, and both are much faster than size for all vectors.

  3. "Don't use the profiler, use tic/toc". True, the profiler includes the time it takes to run itself, but it's still the easiest way to identify bottlenecks. For exact timing however, don't use tic/toc. timeit was introduced in Matlab R2013b, and is much better than tic/toc. If you are using an older version, the function is available at the File Exchange. tic/toc is also dangerous, because it will reset the timer if tic is called inside another function. Have a look at this question on SO from three years ago.

Added benchmarking results
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34

Timing thisTime in Octave show thatshows the vectorized codeimprovement, as a function of vector size. For vectors with only ten elements, the execution time is 4 times faster than25% of the original onecode.

f = @()original_code(vector);
g = @()vectorized_code(vector);
timeit(f)
ans = 0.0019931
timeit(g)
ans = 4.9369e-004

enter image description here

Timing this in Octave show that the vectorized code is 4 times faster than the original one.

f = @()original_code(vector);
g = @()vectorized_code(vector);
timeit(f)
ans = 0.0019931
timeit(g)
ans = 4.9369e-004

Time in Octave shows the improvement, as a function of vector size. For vectors with only ten elements, the execution time is 25% of the original code.

enter image description here

added 223 characters in body
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
Loading
added 267 characters in body
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
Loading
added 797 characters in body
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
Loading
added 110 characters in body
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
Loading
Source Link
Stewie Griffin
  • 2.1k
  • 1
  • 18
  • 34
Loading
lang-matlab

AltStyle によって変換されたページ (->オリジナル) /