Fix acf and acvf default handling

[duffee]

Test fails because CALC is undefined (I blame my knowledge of PP) I'm still going through PP docs and the Book, but putting this PR up in case it's an easy to correct obvious mistake. I added a int h; to avoid warnings about an undefined variable. I'll keep looking at it.

Report

#     Module  Want  Have Where                                                      Howbig
#     ------ ----- ----- ---------------------------------------------------------- ------
#     PDL    2.099 2.104 /home/boyd/devel/local/lib/perl5/x86_64-linux-thread-multi   6835
#     Test::PDL  0.21     0.22 /home/boyd/devel/local/lib/perl5/x86_64-linux-thread-multi  14974

t/ts.t ... Can't load '/home/boyd/perl/PDL/github/PDL-Stats/blib/arch/auto/PDL/Stats/TS/TS.so' for module PDL::Stats::TS:
/home/boyd/perl/PDL/github/PDL-Stats/blib/arch/auto/PDL/Stats/TS/TS.so: undefined symbol: CALC at /usr/lib64/perl5/DynaLoader.pm line 206.
 at t/ts.t line 5.

message from PR #35 for reference

Changing or-assignment to defined-assignment so that acf(0) and acvf(0) return a single value as expected, not all values which is the default behaviour when no argument is given. plot_acf was not affected by the bug, but there's no current test for it.

The definition of acf is that it is the normalization of acvf using the value of the acvf with no lag.
This commit makes it behave as described in docs for TS.pm

[duffee]

Since CALC was only added in 2024 (2.088), there's not a lot written about it. Sounds like I should write a blog about it ;)

This is my understanding:
CALC is a feature available in Pars (see 2.3.2 Basic pp_def keys) that lets you calculate a variable's dimension when the pp function gets called instead of using RedoDimsCode. (only mentioned 4 times in PDL/PP.pod)

when debugging, set $::PP_VERBOSE = 1; in the file and wade through output for every pp_def in that file :-0
redirect the output to a file make > pp_debug.txt and search for your function name.

[duffee]

with the Pars set to use CALC like this (and not in the body of Code like it currently is in the PR)

pp_def('acvf',
  Pars  => 'x(t); [o]v(h=CALC($SIZE(hin) < 0 ? $SIZE(t)-1 : $SIZE(hin)))',
  OtherPars => 'int lag => hin',
  OtherParsDefault => { lag => -1 },

test fails with

PDL::Ops::divide(a,b,c): parameter 'b': Mismatched implicit broadcast dimension 0: size 5 vs. 0
There are 3 PDLs in the expression; 1 broadcast dim.
   PDL IN EXPR.    BROADCAST DIMS
   #  0 (normal):        5
   #  1 (normal):        0
   #  2 (null)
 at lib/PDL/PP.pm line 1450.
	PDL::overload_divide(PDL=SCALAR(0x564315b75580), PDL=SCALAR(0x564315ba3770), "") called at t/ts.t line 15

[mohawk2]

Glad you've found the docs for =CALC. It will be helpful if you can PR a doc update!

Please change the OtherPars so the variable is still an IV, and remove the => hin like I recommended; just use $COMP(lag) directly.

I've noticed that the values in the PMCode add 1 to $h, so you'll need to adjust the =CALC to account for that. The dim is ending up as zero because that's not been done right yet, whereas if it were 1, that would then get "inflated" by the broadcast engine to 5 to match.

[duffee]

I've got $a->acvf($n) to work for defined $n, but $a->acvf() throws

Usage: PDL::acvf(x,[v],lag) (you may leave [outputs] and values with =defaults out of list)

so I must not have the default correct. Hmmm.
Edit: That message croaks
unless $only_one || $argorder || ($nmaxonstack == keys(%valid_itemcounts) + $xs_arg_cnt);
Checking ArgOrder ... and it works! ArgOrder => 1,

Does this make sense/look right? If so, I'll change the acf function to match.

It was nice to find that a matrix works like sumover would and that asking for more lags than values gives a reasonable answer.

[mohawk2]

Simplifying the code by using features that remove the need for PMCode is great! Some more tweaks requested :-)

[coveralls]

coverage: 73.792% (-0.3%) from 74.043% — duffee:fix-acf-acvf into PDLPorters:master

[duffee]

Ok, final version now (fixed my bug). I've made the same changes to acf() and all the tests pass.

I would have organized all the test blocks in t/ts.t into subtests but I thought that might be going too far. I've used a note for the comment on higher order pdls. I hadn't asked whether that is consistent with the test style. I'll wait on any new requested changes and then squash the commits.

BTW, out of curiosity, what's the reason for getting rid of the PMCode sections? Is it a performance hit?

And thank you so very much for append. I was searching and wracking my little brain for that. The dummy example helped with my understanding of how that works.

[mohawk2]

Is this just a comment that PDL's broadcasting exists and works? Please remove this unless there is actual value added, which I don't see yet.

[duffee]

converted to a comment for maintainers saying what is being tested and why it is written the way it is. ie I didn't know before writing the test that the acvf of (0 .. 9) == acvf of (10 .. 19) and it tells the reader that this works like sumover, not sum.

[mohawk2]

That sort of thing belongs in the docs, not in a bit of invisibly-output stuff in a test. Thanks for removing it. By the way, as you know, your doc improvements are also much appreciated!