Re: savevtk

[Philip Nienhuis]

Levente Torok wrote:
> Dear Philip, Carlo and octave-dev,
>
> I had a chance to polish things according to your commands.

Thanks, I'll have a look at them.

> However I have a few questions.
>
> Many of the codes do not fit into this scheme but I can accept that
> this is guideline for the future.
>
> > - 2 spaces rather than tabs (but I still use tabs in my own scripts so I
> > don't mind this particular one)
> >
> > - spaces between internal function name and left paren; no space between
> > array name and left paren or bracket
> >
> > - functions end with "endfunction" rather than "return"
> >
> > - appropriate end statements: if- elseif -else - endif / for - endfor /
> > while - endwhile / etc.
>
> Why dont want we write code that maybe used with matlab too?
> This would be a benefit for all I believe.

O sure, I believe that too, OTOH there are good reasons to keep Octave 
style.
For one, and also my main motive: Octave's script language is simply 
superior to that of ML. (It's easy to write e.g., if...end%if and 
for...end%for to retain ML compatibility but Octave's parser can more 
reliably check code if we follow to the above style suggestions.)
Other -probably more ideological- reasons have been brought up 
repeatedly by Octave developers in the help-octave and 
octave-maintainers mailing lists.

From a more practical point of view, code that is meant to be run 
unchanged under ML doesn't seem to belong in the io package in the 
"main" tree, but rather in some package in the "extra" tree.
See here:
   http://octave.sourceforge.net/developers.html
section "Where does your code belong?"

Unless explicitly instructed otherwise with sufficiently convincing 
reasoning I tend to stick to these guidelines. (Though I violated this 
already by provisionally committing a first versions of your scripts to 
svn, assuming updated and more compliant versions will follow.)

> > - spaces after commas in argument lists (e.g.,  "(a, b, c)" instead of
> > "(a,b,c)")
> >
> > - are you sure your function shouldn't return some value, for successful
> > writing or not? I see no write error catching structures at all
> >
> > - I think you should check for the return value of fopen and perhaps also
> > fprintf to see if the calls succeeded (hint: fprintf returns the number of
> > characters written).
> > A try-catch or unwind_protect construct could be around the fprintf
> > statements to maybe avoid dangling file pointers. Upon errors and ensuing
> > break out of savevtk....() the output file will probably be closed anyway
> > but personally I like more graceful solutions.
> >
> > - comments start with # rather than %
> Why is this if we support multiple type of commenting styles?

See above.

> > - in the Updates: section in the headers, please add some short description
> > of what the update was all about
> >
> > - The copyright sections in the headers seem incomplete
> >
> > - in savevtk.m an empty line is needed between the copyright statement and
> > the texinfo header
> >
> > - the texinfo headers seem a bit odd to me. Have a look at e.g. the Excel or
> > OpenOffice.org scripts for examples
>
> Where are these?

In the io package. But if you use the octave-3.2.4 MingW binary for 
Windows, it is easier to just do from Octave:
  edit newfile.m
and you'll probably have a nice template.

> > - would a test case or some test section be useful?
> I don't know the syntax of the example however it could be something like:
>
> n=30;
> X=zeros(n,n,n);
> for x=1:n
>    for y=1:n
>      for z=1:n
>        X(x,y,z)=1/sqrt( x*x + y*y + z*z );
>      endfor
>    endfor
> endfor
> Maybe someone can write the same code with matrix arithmetic I did
> tackle with this.
>
> > But OK, maybe I am just too picky....

According to Carlo, to some extent I am.... :-) (see his last mail/post 
to me)

If you want your scripts in the io package I could adapt the code 
(-style) a bit further for you (and send them to you for review before 
committing them), but if you want to retain ML compatibility I suppose 
your scripts perhaps should rather be elsewhere in the svn trunk.

Let me know what you want.


Thank you Levente.

Philip