bugGNU Octave - Bugs: bug #46897, nchoosek accepting non-numeric...

 
 

bug #46897: nchoosek accepting non-numeric input

Submitted by:  Luis Mendo <lmendo>
Submitted on:  Thu 14 Jan 2016 02:49:47 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: NoneAssigned to: None
Originator Name: Luis MendoOpen/Closed: Open
Release: 4.0.0Operating System: Microsoft Windows

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 11 Mar 2016 08:56:45 PM UTC, comment #6:

@Lachlan: sorry I'm rather busy with other projects these days. If you feel like cleaning up the format yourself, please do so. Thanks for your time in any case

Luis Mendo <lmendo>
Fri 26 Feb 2016 02:47:44 PM UTC, comment #5:

Luis, did I overwhelm you with the list of coding standards in the previous comment? Please let me know if you'd like a hand with any of this.

Lachlan Andrew <lachlan>
Project Member
Mon 18 Jan 2016 02:41:31 AM UTC, comment #4:

Thanks Luis. I think I understand now.

This code looks very clean.

To increase its chances of getting included in Octave, follow the guidelines at https://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html. I'd recommend the following changes:

0. Make this a drop-in replacement for nchoosek. In particular, it must handle the (usual) case that the first argument is a scalar, in which it should return the binomial coefficient.

1. Include a GNU licence at the start, listing you as the copyright holder. See existing Octave .m files for a template

2. Validate the input before using it. What if this is called with only one input argument? If it is called with 3 it will work, but should report an error. Follow the example text when you give too many arguments to existing Octave functions.

3. Put a space before () that are a function call, but keep the () and {} for a variable reference with no space, as you have now.

4. Add a documentation string to the start, so that "help nchoosek" describes the new functionality. Look at some existing .m files for the (texi?) markup.

5. Check if there is much performance penalty for the (usual?) case that the input is numeric. If so, you may want to have an "if" use the builtin nchoosek directly for arguments that it handles. It may be best to combine this with step 0.

Thanks in advance for your effort. I hope that this makes it into a future release.

Lachlan Andrew <lachlan>
Project Member
Fri 15 Jan 2016 07:20:38 PM UTC, comment #3:

I'm sorry for the confusion. By `unique` I meant `nchoosek`. It does make sense. For example, `nchoosek(1:3,2)` currently produces

My proposal is to have `nchoosek('abc',2)` produce

and to have `nchoosek('{'a', 2, 'ttt'},2)` produce

ans =
'a' [ 2]
'a' 'ttt'
[2] 'ttt'

like Matlab does. My attached function achieves that.

Luis Mendo <lmendo>
Fri 15 Jan 2016 05:14:40 AM UTC, comment #2:

Luis, you talk about using 'unique', but the code uses nchoosek, which matches the subject and first paragraph.

I can see how using unique on a more general type makes sense, but I can't see how nchoosek makes sense for non-numeric values.

Could you please clarify what you are proposing?

Lachlan Andrew <lachlan>
Project Member
Thu 14 Jan 2016 02:51:13 PM UTC, comment #1:

Sorry for attaching twice. It's the same file

Luis Mendo <lmendo>
Thu 14 Jan 2016 02:49:47 PM UTC, original submission:

I've noticed that Octave's nchoosek function requires the first input to be a scalar or a numeric vector. In Matlab it can be a char vector (i.e. string) or a row cell array. I think this is good because it extends functionality.

It's very easy to generalize Octave's `unique` to do this, using a workaround: call builtin `unique` with a numeric vector and then use the result that to index into the original input. I'm attaching my workaround function.

Luis Mendo <lmendo>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #36029:  nchoosek_comp.m added by lmendo (177B - text/plain)
file #36028:  nchoosek_comp.m added by lmendo (177B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by lachlan (Posted a comment)
  • -unavailable- added by lmendo (Submitted the item)
  • -unavailable- added by lmendo
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 14 Jan 2016 02:50:26 PM UTClmendoAttached File-=>Added nchoosek_comp.m, #36029
    Thu 14 Jan 2016 02:49:47 PM UTClmendoAttached File-=>Added nchoosek_comp.m, #36028
      Carbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1