patchGNU Octave - Patches: patch #8856, add function reducevolume

 
 

patch #8856: add function reducevolume

Submitter:  None
Submitted:  Wed 13 Jan 2016 02:50:24 PM UTC
   
 
Category:  Core : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 09 Aug 2016 07:41:26 PM UTC, comment #10: 

I cleaned up the patch for reducevolume and pushed it here (http://hg.savannah.gnu.org/hgweb/octave/rev/01ba6ebc52e4).

Rik <rik5>
Group administrator
Mon 18 Jul 2016 05:52:02 PM UTC, comment #9: 

The attached patch should apply cleanly on top of file #37935 in patch #8864 "smooth3".

(file #37936)

Markus Mützel <mmuetzel>
Group administrator
Wed 06 Jul 2016 08:19:24 PM UTC, comment #8: 

Lachlan, attached is the changeset with the added linewraps.

(file #37740)

Markus Mützel <mmuetzel>
Group administrator
Wed 06 Jul 2016 11:25:13 AM UTC, comment #7: 

Markus, if you would like this considered for 4.2.0, could you please wrap lines at 72 columns (or perhaps up to 79) except the typedefun lines, and create a Mercurial changeset?

Lachlan Andrew <lachlan>
Tue 19 Jan 2016 04:52:52 PM UTC, comment #6: 

smooth3.m is patch #8864
isocaps.m is patch #8872

I added the "see also" to the help text (probably forgot to save before submitting the prior patch), added a check of the input/output argument count, and refactored the tests a little bit to clean them up.

Other than that, I only cleaned the code a little wrt the prior patch.

I have still some doubts wrt the other two functions (please, see my comments there), but as soon as those are rectified and they meet the expected quality, the three files could be added to the repo.

(file #36095)

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jan 2016 05:44:19 PM UTC, comment #5: 

Thanks for appreciating my nitpicking :-)

Should we wait for smooth3.m to be provided?
It can take some time before your contributions are added to the repo, so maybe this isn't a bad idea. Same goes for isocaps, if you want to.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Jan 2016 08:48:45 AM UTC, comment #4: 

Thank you for spotting the issue with the coding style I still overlooked.

I don't mind the spelling of my family name. The only reason why Germans use umlauts is probably to cause problems with different code pages ;-)

I noticed after the last submission that most examples do not use the syntax with @var{x}. So I changed that.

As a following patch, I almost got a version of smooth3 working. So I already added that function and isosurface to the "see also" list. Is this OK?

I might continue with taking a look at isocaps. However, I doubt I can help with camlight and lighting as I suspect that they have to be implemented with the rendering engine and cannot be done solely with Matlab functions. (At least the lighting part...)

(file #36076)

Markus Mützel <mmuetzel>
Group administrator
Sat 16 Jan 2016 10:57:02 PM UTC, comment #3: 

Marvellous job! Thanks very much

As to coding style, I've only managed to spot and change the "~" operator in lines 137, 144 and 151 to "! ".

Your family name didn't come through - I hope you don't mind I spelled it as Muetzel in the copyright line?
As your name is in the function, I suppose you do not mind to be mentioned in the list of contributors?

I tried the example on the Mathworks site; that works well as far as other missing functions go (smooth3, isocaps, camlight, lighting); the resulting picture resembles the shape on the TMW page quite well.
That's as far as I can test the validity and proper operation of the function.

I'll cc an other core devs about his opinion on where to put it (I'm merely a contributor with push credentials).

@Rik:
If you agree, I can make a cs & push it to default in the next week or week after.  Where should it be put? plot/util? image? geometry? statistics?


(file #36058)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 14 Jan 2016 11:40:13 AM UTC, comment #2: 

Thank you for your prompt and kind comments.

Like you suggested, I added a some more help text with simple examples for the usage. I hope it is understandable and not too confusing.

Additionally, I added some tests for the different usage scenarios (number and format of input and output arguments) and other tests triggering each of the errors that can be reached. I did not find out how to do error test on more than one line. Thus, the lines for these tests got a little "long-ish".

The subfunction for input validation checks for 3 or 6 input arguments because the number of output arguments of the main function is passed as additional first parameter. The main function itself must be called with 2 or 5 arguments (like the original one in Matlab).

I hope I addressed all occurrances of code style issues.

Please, let me know how to proceed.


(file #36026)

Markus Mützel <mmuetzel>
Group administrator
Wed 13 Jan 2016 11:09:58 PM UTC, comment #1: 

Quite (=very) good for a first contribution! Thank you.

Nevertheless :-)  some points I noted in a ~2 minute scan:

Texinfo header:
---------------

  • The first deftypefnx should without the trailing x: deftypefn


  • The first sentence of the help is OK, but what is lacking is the rest of the help text, i.c., information on the required and the optional input parameters and what output arguments the function returns. Maybe even usage examples? Octave users shouldn't have to resort to the Matlab on-line help, should they :-)

E.g., only by perusing the code I noted somewhere that the function requires either three or six input arguments (according to Matlab it should be 2 or 5). That should be in the texinfo help text, to be displayed when doing:

help reducevolume

Code style (-nitpicking):
-------------------------

  • We prefer ! negation operators rather than ~

In L.67 I read "if (numel (r) ~= 3)". It should rather be != or maybe "if ! (numel (r) == 3)"

  • On L.71 "if any (r(:) < 1 | r(:) != fix (r(:)))": The entire if condition (any ...... ) should be enclosed in parentheses.


  • We start comments with double ## rather than single # or %


  • L.87 "x = 1:size(data, 2);" size() is a function so we prefer a space between size and the left paren: "x = 1:size (data, 2);"


General (important!):
---------------------

  • I see no tests. Can you provide tests, also for input validation? For code & style examples see a.o., strread.m



Philip Nienhuis <philipnienhuis>
Group Member
Wed 13 Jan 2016 02:50:24 PM UTC, original submission:  

Hello,

here is my first patch for Octave adding the function reducevolume.
I am new to Octave but quite familiar with Matlab.
I try to get my Matlab projects to run with Octave. As there are still some functions missing in Octave that exist in Matlab, I try and help to get it more complete.
I would really like to see transparency in plots. But as this is out of my abilities, I would like to contribute a function that I use quite regularly.
The function reducevolume is not too complex, so I chose it for a first patch.
I hope this is the right way to submit it.
I tried to imitate the style of other functions provided with Octave.
The function seems to give the same results as in Matlab (at least as far as I tested).
Please, let me know what needs to be changed.

Best regards
Markus Mützel

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #37936:  reducevolume_v2.patch added by mmuetzel (13KiB - text/x-diff)
file #37740:  reducevolume.patch added by mmuetzel (12KiB - text/x-diff)
file #36095:  reducevolume.m added by mmuetzel (10KiB - text/plain)
file #36076:  reducevolume.m added by mmuetzel (11KiB - text/x-objective-c)
file #36058:  reducevolume.m added by philipnienhuis (11KiB - text/x-objective-c)
file #36026:  reducevolume.m added by mmuetzel (11KiB - application/vnd.wolfram.mathematica.package)
file #36014:  reducevolume.m added by None (4KiB - application/vnd.wolfram.mathematica.package)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-09 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2016-07-18 mmuetzel Attached File- Added reducevolume_v2.patch, #37936
    2016-07-07 lachlan CategoryNone Core : new function
    2016-07-06 mmuetzel Attached File- Added reducevolume.patch, #37740
    2016-01-19 mmuetzel Attached File- Added reducevolume.m, #36095
    2016-01-18 mmuetzel Attached File- Added reducevolume.m, #36076
    2016-01-16 philipnienhuis Attached File- Added reducevolume.m, #36058
        Carbon-Copy- Added rik
    2016-01-14 mmuetzel Attached File- Added reducevolume.m, #36026
    2016-01-13 None Attached File- Added reducevolume.m, #36014

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code