octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GSoC interval package] Code Review


From: Oliver Heimlich
Subject: [GSoC interval package] Code Review
Date: Sat, 17 Jun 2017 00:29:09 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

Hi Joel,

I have looked at your latest changes.

- Line continuation characters missing here after reformatting?
https://sourceforge.net/u/urathai/octave/ci/b25fbf0ad324af2e27d02379ce58983e532a9f33/tree/inst/@infsup/infsup.m#l277

On 16.06.2017 13:38, Joel Dahne wrote:
> Oliver Heimlich writes:
>> On 14.06.2017 12:05, Joel Dahne wrote:
>>> I have begun the work on adding support for constructing and printing
>>> N-dimensional interval arrays.
>>
>> I have reviewed and merged your first bulk of work.  Review comments:
>> …
> I have fixed all of the above problems. Actually "sprintf" can handle
> vectors in a nice way, so it was even easier than I thought.

Nice!

>> Revision 904 (size.m): You should change the texinfo macro @defmethod
>> into @deftypemethod and put the output arguments into curly braces.
>> Also you should format the new help text with @command{size} macros
>> (instead of plain quation marks).

> I have done these changes. I will have to learn a bit more about texinfo
> to understand what I'm doing though. Is it for 'make html' that the
> texinfo is used? Because from what I can tell it is not visible in the
> help text inside Octave.

Found more Texinfo pitfalls:

In size.m, when you change from @defmethod to @deftypemethod it is
necessary to also update the corresponding @end macro.  You can think of
\begin{x} … \end{x} in Tex.

In infsup.m and disp.m please make sure that you put 2 spaces between
two sentences which end and start on the same line.  Otherwise Texinfo
might conclude that the period is used for an abbreviation and not to
end the sentence.

More background information:
https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Not-Ending-a-Sentence.html

>>> disp.m: When printing an array with more than 2 dimensions I have
>>> followed the normal Octave style. It looks like
>>>  ans(:,:,1) =
>>>
>>>    [0]   [0]
>>>    [0]   [0]
>>
>> Two small observations:
>>
>>  1. The matrix label “ans(:,:,…)” should not be indented.
>>  2. Before the matrix label you need an additional blank line (not
>> before the first one). Remark: disp.m currently only supports “format
>> loose”, where there are blank lines between matrix labels and matrices.
>> Feel free to add support for “format compact”, but I don't know how to
>> properly detect the current display mode.
>>
> 
> I have fixed the observations you made! Have not added support for
> "format compact" though, I don't know how to properly detect this
> either.

Ok.  Rik has provided a way to detect the current format.  However,
since this is a feature yet to come in a future release of Octave, we
can delay this topic for now.

Best
Oliver



reply via email to

[Prev in Thread] Current Thread [Next in Thread]