[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Octave-patch-tracker] [patch #7929] Partial Differential Equation Solve
From: |
Jordi Gutiérrez Hermoso |
Subject: |
[Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions |
Date: |
Wed, 06 Feb 2013 21:49:35 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0 Iceweasel/18.0.1 |
Follow-up Comment #7, patch #7929 (project octave):
Okay, so I'm starting to think about importing your code into Octave...
I won't comment on the mathematical aspects of it, so please be patient with
the apparently trivial remarks I have to make about its superficial
appearance.
Why are you putting 1; at the top of files, in effect using script files and
no function files?
Should this be a package or a core function?
There are many stylistic issues to also discuss. We prefer a different Octave
house style to Matlab style. Read some of Octave's source code to glean this
style and this:
http://www.gnu.org/software/octave/doc/interpreter/Octave-Sources-_0028m_002dfiles_0029.html
I see the following problems with your code:
* Using inline("foo(a,b)", "a", "a") instead of @(a,b) foo(a,b)
* No explicit end statements (endfor, endif, endwhile...)
* % for comments instead of ##
* No space between function names and opening bracket: sin (x), not sin(x)
(but note, no space for indexing, just a visual cue to distinguish function
calls from indexing).
* Single-quoted strings. Except in some cases, we generally prefer
double-quoted strings.
* Exceedingly long lines. Lines should not be more than 80 characters wide.
Note that unlike Matlab, Octave does not require continuation characters if
there's an opening bracket that hasn't been closed on this line yet. Use this
feature to your advantage when breaking up lines.
* Using ~= instead of the more common !=
* No spaces after commas.
* Inconsistent use of tabs and indentation. Just don't use any tabs at all and
use two-space indentation.
* Windows newlines (/r/n). We use Unix newlines (just /n).
Here is a more profound problem: I see a lot of code duplication here. Code
duplication is very bad. In the future as we need to maintain this code, code
duplication means that we have to hunt down all instances of the same code and
patch it in the same way. Please move as much common code as you can into
common functions shared by others. If necessary, use private directories and
or subfunctions.
Another problem, I see some very trivial loops that can easily be replaced.
Instead of
function [u] = osb22ASol( x , t)
C1 = 0.1;
C2 = 1.0;
P = 1.1;
for i=1:length(t)
for j=1:length(x)
if x(j)<0
u(i,j) = log( C1 * x(j) + t(i) + P);
else
u(i,j) = log( C2 * x(j) + t(i) + P);
end
end
end
end
this could easily be
function [u] = osb22ASol(x , t)
P = 1.1;
C = ifelse (x < 0, 0.1, 0.2);
u = log (C*x(:) + t(:)' + P)
endfunction
This is just from a quick glance of the code, sorry I didn't delve deeper into
the actual mathematics of it.
_______________________________________________________
Reply to this item at:
<http://savannah.gnu.org/patch/?7929>
_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions,
Jordi Gutiérrez Hermoso <=
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Jordi Gutiérrez Hermoso, 2013/02/06
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Jordi Gutiérrez Hermoso, 2013/02/06
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Mark Wistrom, 2013/02/06
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Mark Wistrom, 2013/02/11
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Mark Wistrom, 2013/02/12
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Dongxu, 2013/02/14
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Mark Wistrom, 2013/02/14
- [Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions, Mark Wistrom, 2013/02/14