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

 
 

bug #46897: nchoosek accepting non-numeric input

Submitter:  Luis Mendo <lmendo>
Submitted:  Thu 14 Jan 2016 02:49:47 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Luis Mendo Open/Closed:  * Closed
Release:  * 4.0.0 Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 17 Apr 2022 01:40:46 PM UTC, comment #8: 

Going through old bugs related to nchoosek, found this one that has been fixed already. Closing as fixed.

Arun Giridhar <arungiridhar>
Group Member
Sat 02 Apr 2022 02:47:35 PM UTC, comment #7: 

This old bug report is no longer relevant and should be closed. nchoosek accepts char inputs now.

Anonymous
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>
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>
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


   1   2
   1   3
   2   3

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


ans =
    ab
    ac
    bc


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>
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 Files:
   
   
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
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by lmendo (Submitted the item)
  • -email is unavailable- added by lmendo
  •  

    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 group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-04-17 arungiridhar StatusNone Fixed
        Open/ClosedOpen Closed
    2016-01-14 lmendo Attached File- Added nchoosek_comp.m, #36029
    2016-01-14 lmendo Attached File- Added nchoosek_comp.m, #36028
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code