patchGNU Octave - Patches: patch #8801, add support for the histogram and...

 
 

patch #8801: add support for the histogram and histcounts functions

Submitted by:  None
Submitted on:  Fri 20 Nov 2015 05:47:40 PM UTC  
Votes:  1  
 
Category:  Core : new function Priority:  5 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Open

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)

Fri 05 Oct 2018 06:34:00 PM UTC, comment #12:

Hey all, I recently wrote my own implementation for the histcounts function. If all goes well I also have a histogram implementation in the works.

I'm giving you the raw files since I'm not sure where to put the C++ or how to integrate it into the build system. The principal files are histcounts.m and histcountsbin.cc.

The C++ will probably need some cleanup from my lack of experience. It will take a little while to compile because it accommodates for the integer types.

I've included some testing functions in the tarball. Use the paralleltest function to run them. You'll need pkg:parallel and MATLAB installed.

(file #45153)

Gene Harvey <gharveymn>
Sat 08 Sep 2018 03:27:12 PM UTC, comment #11:

Thanks for the Cc. I indeed never saw the messages and forgot to check by myself here. I will look into the reported issue.

Lars Winterfeld <lawilog>
Fri 07 Sep 2018 05:39:04 PM UTC, comment #10:

Very well.

So, as Lars has never been CC'd I guess your comment #1 also never reached him.
I've put his email (from the patch) in the cc list.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Sep 2018 08:39:31 PM UTC, comment #9:

Lars has not responded since the original posting in 2015 and is not cc'd on this bug report, so keep that in mind while waiting for a response from them.

If we don't hear back, and you (Philip) want to pursue getting these functions in shape for inclusion in Octave, I would recommend reverting Lars' copyright to 2015 only for the original submission, and add a copyright for yourself along with whatever range or set of years you have made any significant changes to these functions.

Setting the status back to "postponed" for now, more work needed.

Mike Miller <mtmiller>
Project Administrator
Thu 06 Sep 2018 06:45:14 PM UTC, comment #8:

No, this has nothing to do with importdata or what the random seed is. I have started Octave multiple times (different random initial state each time) and run 'test histcounts' alone multiple times, and it always fails randomly when the test is repeated. Like I said, the test is buggy. Either the random vector that it is operating on is outside of its expected range (doubtful), or the alogrithm inside of the histcounts function is missing some corner case that the random vector is exercising (more likely).

Mike Miller <mtmiller>
Project Administrator
Thu 06 Sep 2018 06:35:17 PM UTC, comment #7:

Mike, thanks for the review.
I just kept the patch "alive", I have used the functions a few times and it seems useful to me.
Where should histogram.m go?

I only ever saw a FAIL for histcounts.m during make check or _run_test_suite_ but "test histcounts" on its own has always, consistently, PASSed here; except for bug #54622 as it turn out now.
If that is related to random numbers as you suggest I tend to suspect there may be an issue with the seed, exposed by the way the test suite (or importdata.m) operates.

Let's wait to see if the OP (Lars W) will update the patch, if not I might do it at some time.
Note that I've already amended quite a few code style issues (e.g., "%" comment lines.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Sep 2018 06:02:04 PM UTC, comment #6:

The histcounts function should be in Octave, in the scripts/statistics directory just as the patch makes it.

The summary line of the doc string for histcounts should probably mention the input variable X (e.g. "Compute histogram bin counts for the data array @var{x}").

The second paragraph starting with "Input arguments: X ..." and the last paragraph starting with "Output arguments: N ..." do not fit with Octave doc string style.

There are some code style issues (extra or missing space before or after parentheses, missing parentheses on "switch" argument, missing parentheses around complex logical conditions). There are a couple of trailing spaces in the doc string block that should be deleted.

The test suite is buggy, running it repeatedly causes failures in some of the tests that rely on random input data. I haven't looked into it to see whether this is a bug in the test formulation or a bug in the histcounts function itself.

Mike Miller <mtmiller>
Project Administrator
Tue 17 Jul 2018 07:05:51 PM UTC, comment #5:

Again an updated patch, relative to hg id d1216d085280 (July 18 2018)
Unsure if histcounts.m should be in core octave or in the statistics package

(file #44574)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 21 Apr 2018 08:39:01 PM UTC, comment #4:

Attached is an updated patch.
I've fixed it cf. comment #2 and added the missing module.mk entries to include them in dist archives + removed them from__unimplemented__.m
NEWS entries are not added yet.

I've been including this patch for a looong time now, I think they can be incorporated straight away; maybe except histcounts.m as that may live in the OF statistics package?

(file #43985)

Philip Nienhuis <philipnienhuis>
Project Member
Sun 20 Nov 2016 08:26:57 PM UTC, comment #3:

In addition, I think "isreal" => "isnumeric" in L.203

Philip Nienhuis <philipnienhuis>
Project Member
Sun 20 Nov 2016 08:25:57 PM UTC, comment #2:

Q:
In histcounts I see this stanza in L.201-205:

Shouldn't the if statement check if "binwidth" rather than "binmethod" is real?

(looks like a copy-paste from he switch clause above)

Philip Nienhuis <philipnienhuis>
Project Member
Tue 09 Feb 2016 11:40:39 PM UTC, comment #1:

Lars, thanks for taking the time to write and contribute these functions to Octave!

This submission looks really good, I have not yet tried or tested the new functions, and haven't checked the Matlab description of these functions, but they look well done, including unit tests and proper coding style. Still needs further review and testing for functionality.

Please assign the copyright to yourself in the file header, it should read "Copyright (C) 2015 Lars Winterfeld" (and the "Author" line is not really needed once that change is made).

This patch should also add the new scripts to the module.mk files in the appropriate directories, and add a mention of these new functions to the top-level NEWS file for the future 4.2 release.

Mike Miller <mtmiller>
Project Administrator
Fri 20 Nov 2015 05:47:40 PM UTC, original submission:

more sophisticated version of histc and hist.
not yet implemented: category cell arrays as input.

Those are the two new histogram function in Matlab >= 2014 (or so). I used ideas from both the old hist and histc functions.

However, if the bins are uniform spaced (which is often the case, also if automatically determined), then the bin indexes are now calculated in O(N) instead of O(N*M), where N is the number of data points and M the number of bins. If not uniform spaced, I use the lookup function, which finds the indexes in O(N*log(M)), because the bin edges are sorted.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45153:  testing.tar.gz added by gharveymn (12KiB - application/gzip)
file #35507:  histogram+histcounts.hg-export.diff added by None (16KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gharveymn (Updated the item)
  • -email is unavailable- added by lawilog (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by eflower3 (Voted in favor of this item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-05 gharveymn Attached File- => Added testing.tar.gz, #45153
    2018-09-07 philipnienhuis Carbon-Copy- => Added -email is unavailable-
    2018-09-06 mtmiller StatusReady For Test => Postponed
    2018-07-17 philipnienhuis Attached File- => Added patch_8801_histogram+histcounts_v3.cset, #44574
    2018-04-21 philipnienhuis Attached File- => Added patch_8801_histogram+histcounts_v2.cset, #43985
        CategoryNone => Core : new function
        StatusNeed Info => Ready For Test
    2016-10-17 eflower3 Carbon-Copy- => Added eflower3
    2016-02-09 mtmiller StatusNone => Need Info
    2015-11-20 None Attached File- => Added histogram+histcounts.hg-export.diff, #35507

    Back to the top


    Powered by Savane 3.3