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

 
 

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

Submitter:  None
Submitted:  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
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 19 Mar 2024 05:14:57 PM UTC, comment #19: 

was reminded of this patch and going to check it out, but it looks like somehow savannah has dropped the comment #12 compiled version of these functions. Will look at the last patch by philip for m-code versions and see where it stands.

Nicholas Jankowski <nrjank>
Group Member
Wed 11 Aug 2021 07:56:06 PM UTC, comment #18: 

@Nick:
Great if you'd update histcounts & histogram. I second the preference for an .m file version for easier maintenance and I also agree that a compiled version would require "a wider audience".

Once you get it fixed we'd have to decide on where to push it to. Mike wanted core Octave, but it's as easy to put in in the OF statistics package. I've been adding quite a few contributions there lately so it's about time to make a new release in which histcounts & histogram could be included as well.

Opinions?

Philip Nienhuis <philipnienhuis>
Group Member
Tue 10 Aug 2021 11:51:27 PM UTC, comment #17: 

ok, a simple 'mkoctfile histcountsbin.cc' was all that was needed to compile the garveymn version.  the two quick tests below match matlab output.

unless you think histcounts is going to get used so often it warrants compiling, I still like the idea of having a function like this in a more accessible m file.  I'll look at the algorithms and see if it's obvious where the issues are. Also, the docstring and BISTs could use some rework.

Nicholas Jankowski <nrjank>
Group Member
Tue 10 Aug 2021 07:59:06 PM UTC, comment #16: 

no since his requires compilation I didn't get a chance to try it out.  if i have some time later i might peek at the algorithms and see if i can lift any tweaks from there to fix Lars' code.  (otherwise if it's sufficiently standalone I could probably just mkoctfile his code and play from there, but I don't have a C++ IDE to step through, so prefer to live in m-space)


Nicholas Jankowski <nrjank>
Group Member
Tue 10 Aug 2021 04:38:36 PM UTC, comment #15: 

Just FTR, it's not mine but rather Lars' contribution. I just kept them up-to-date.
@Nick: did you also try the versions contributed by garveymn (comment #12)?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 09 Aug 2021 11:28:00 PM UTC, comment #14: 

running through a few examples from the matlab help, i think at least Philip's version is not quite compatible yet. (i'm not set up to compile octave to test, so can't say whether the c version works) e.g.,:

Matlab doc:

X = [2 3 5 7 11 13 17 19 23 29];
[N,edges] = histcounts(X,6)
N = 1×6

     2     2     2     2     1     1

edges = 1×7

         0    4.9000    9.8000   14.7000   19.6000   24.5000   29.4000


Octave:

X = [2 3 5 7 11 13 17 19 23 29];
[N,edges] = histcounts(X,6)
N =

   3   1   2   2   1   1

edges =

 Columns 1 through 6:

    2.0000    6.5000   11.0000   15.5000   20.0000   24.5000

 Column 7:

   29.0000



Matlab docs:

X = primes(100);
[N,edges] = histcounts(X, 'Normalization', 'probability')
N = 1×4

    0.4000    0.2800    0.2800    0.0400

edges = 1×5

     0    30    60    90   120


Octave:

X = primes(100);
 [N,edges] = histcounts(X, 'Normalization', 'probability')
N =

   0.4400   0.2800   0.2800

edges =

    2.0000   33.6667   65.3333   97.0000




Nicholas Jankowski <nrjank>
Group Member
Mon 09 Aug 2021 11:00:09 PM UTC, comment #13: 

peeking at this patch as it was recently mentioned in bug #60983

it looked like Philip had a fairly complete version of the functions, minus the category array inputs options for both functions, and histogram returning an axes handle vs some new Histogram object (neither are implement in octave yet, correct?)

Is there anything else missing, any options missing that comment #12 histcount covers?  At first glance it seems to have most/all of the name/value pairs accounted for, but i haven't done an exhaustive check. 

I'm not sure what the extra complexity of a c++ version gives. safe to assume pkg parallel/Matlab is only needed for those tests, not the actual function?

function docs for both at:
https://www.mathworks.com/help/matlab/ref/histcounts.html
https://www.mathworks.com/help/matlab/ref/matlab.graphics.chart.primitive.histogram.html

Nicholas Jankowski <nrjank>
Group Member
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>
Group 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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
Sun 20 Nov 2016 08:26:57 PM UTC, comment #3: 

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

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

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

        case "BinWidth"
          binwidth = varargin{++iarg};
          if (! isreal (binmethod))
            error ("histcounts: BinWidth must be followed by a real number");
          endif


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>
Group 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>
Group Member
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 #35507:  histogram+histcounts.hg-export.diff added by None (16KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by nrjank (Posted a comment)
  • -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)
  •  

    There is 1 vote 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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-03-19 nrjank Dependencies- bugs #60983 is dependent
    2021-08-12 jwe Attached File#45153 Removed
    2018-10-19 mtmiller Carbon-CopyRemoved 80942 -
    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.13-f8d8.
    Corresponding source code