[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Octave-patch-tracker] [patch #8872] add isocaps
From: |
Lachlan Andrew |
Subject: |
[Octave-patch-tracker] [patch #8872] add isocaps |
Date: |
Thu, 7 Jul 2016 02:38:02 +0000 (UTC) |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 |
Follow-up Comment #5, patch #8872 (project octave):
Thanks, Markus.
I forgot to mention that we also need to change all docstrings to replace
{Function file} by {}.
Other comments are:
1. Are you sure which_caps can only be a/above or b/before? If not, it may be
better to make "b", "before" a separate case and have "otherwise" throw an
error.
2. I think in-line comments should start with a single "#".
3. It is expensive to building faces and vertices one element at a time like
in the 'if (strcmpi (which_plane, "all"))' since it requires a copy each time
the matrix grows. It would be faster to allocate f1, f2, ..., f5, and then
form [faces; f1; f2; f3; f4; f5]. Similarly, it is faster to build
vertices_grid from row 3 to row 1, or to preallocate it.
4. Several of the lines are too long.
5. It would be good it the docstring didn't assume that the reader knows what
end-caps are.
6. I'm not sure, but I think you can replace
@table @asis
@item @code{above}
by
@table @code
@item above
If so, the latter probably expresses your intent better.
7. __get_isocaps_patches__ seems quite repetitive, which makes maintenance
hard. Is it possible to use the switch statement to choose a small number of
parameters (like which dimension to use), and then have a single isosurface
command? That may end up being less clear (which is also a maintenance
issue).
Hmm... You have submitted lots of patches (thank you!) and it will take a long
time to review them all. Would you like to prioritise your patches, with the
ones you think should be reviewed first?
_______________________________________________________
Reply to this item at:
<http://savannah.gnu.org/patch/?8872>
_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/
- [Octave-patch-tracker] [patch #8872] add isocaps, Lachlan Andrew, 2016/07/06
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/06
- [Octave-patch-tracker] [patch #8872] add isocaps,
Lachlan Andrew <=
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/08
- [Octave-patch-tracker] [patch #8872] add isocaps, Philip Nienhuis, 2016/07/16
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/16
- [Octave-patch-tracker] [patch #8872] add isocaps, Philip Nienhuis, 2016/07/18
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/18
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/18
- [Octave-patch-tracker] [patch #8872] add isocaps, Philip Nienhuis, 2016/07/19
- [Octave-patch-tracker] [patch #8872] add isocaps, Philip Nienhuis, 2016/07/19
- [Octave-patch-tracker] [patch #8872] add isocaps, Markus Mützel, 2016/07/19
- [Octave-patch-tracker] [patch #8872] add isocaps, Philip Nienhuis, 2016/07/19