Skip to content

Fix acf and acvf default handling in PDL::Stats::TS#35

Closed
duffee wants to merge 0 commit into
PDLPorters:masterfrom
duffee:master
Closed

Fix acf and acvf default handling in PDL::Stats::TS#35
duffee wants to merge 0 commit into
PDLPorters:masterfrom
duffee:master

Conversation

@duffee
Copy link
Copy Markdown
Contributor

@duffee duffee commented Oct 8, 2025

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

Tests pass locally with this output

$ prove -Ilocal/lib/perl5 -b t/
t/00-report-prereqs.t .. # 
# Versions for all modules listed in MYMETA.json (including optional ones):
# 
# === Configure Requires ===
# 
#     Module  Want  Have Where                                                    Howbig
#     ------ ----- ----- -------------------------------------------------------- ------
#     PDL    2.099 2.100 /tmp/PDL-Stats/local/lib/perl5/x86_64-linux-thread-multi   6835
# 
# === Build Requires ===
# 
#     Module  Want  Have Where                                                    Howbig
#     ------ ----- ----- -------------------------------------------------------- ------
#     PDL    2.099 2.100 /tmp/PDL-Stats/local/lib/perl5/x86_64-linux-thread-multi   6835
# 
# === Test Requires ===
# 
#     Module     Want     Have Where                                                    Howbig
#     ---------- ---- -------- -------------------------------------------------------- ------
#     Test::More 0.88 1.302195 /usr/share/perl5/vendor_perl                              54209
#     Test::PDL  0.21     0.22 /tmp/PDL-Stats/local/lib/perl5/x86_64-linux-thread-multi  14754
# 
# === Runtime Requires ===
# 
#     Module  Want  Have Where                                                    Howbig
#     ------ ----- ----- -------------------------------------------------------- ------
#     PDL    2.099 2.100 /tmp/PDL-Stats/local/lib/perl5/x86_64-linux-thread-multi   6835
# 
# === Runtime Recommends ===
# 
#     Module                Want    Have Where                       Howbig
#     --------------------- ---- ------- --------------------------- ------
#     PDL::GSL               any missing                                  0
#     PDL::Graphics::Simple  any   1.015 /usr/local/share/perl5/5.38  49691
# 
t/00-report-prereqs.t .. ok   
t/basic.t .............. ok    
t/glm.t ................ ok     
t/kmeans.t ............. ok    
t/ts.t ................. ok    
All tests successful.
Files=5, Tests=337,  3 wallclock secs ( 0.05 usr  0.00 sys +  2.27 cusr  0.06 csys =  2.38 CPU)
Result: PASS

when new tests run with previous version of PDL::Stats::TS, it fails with

t/ts.t ................. 1/? PDL::Ops::divide(a,b,c): parameter 'b': Mismatched implicit broadcast dimension 0: size 6 vs. 10
There are 3 PDLs in the expression; 1 broadcast dim.
   PDL IN EXPR.    BROADCAST DIMS
   #  0 (normal):        6
   #  1 (normal):       10
   #  2 (null)
 at lib/PDL/PP.pm line 1445.
	PDL::__ANON__(PDL=SCALAR(0x55c2e72d2580), PDL=SCALAR(0x55c2e72d2988), "") called at t/ts.t line 15
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 5.

@duffee duffee changed the title Fix acf and acvf default handling Fix acf and acvf default handling in PDL::Stats::TS Oct 8, 2025
@duffee
Copy link
Copy Markdown
Contributor Author

duffee commented May 7, 2026

@zmughal @mohawk2 Can I get a review of this PR, please? It's just a 2 line change with added tests to cover an edge case.

@mohawk2
Copy link
Copy Markdown
Member

mohawk2 commented May 7, 2026

It looks great. However, there is a better way to achieve this! Do you feel like updating acvf so it uses OtherParsDefaults instead, so it can drop the PMCode? There will be other candidates for this as well.

@mohawk2
Copy link
Copy Markdown
Member

mohawk2 commented May 7, 2026

I've just noticed you're PR-ing this from your repo's master branch. Please don't do that going forward.

@duffee
Copy link
Copy Markdown
Contributor Author

duffee commented May 8, 2026

closed by mistake using the github web interface - commits rescued and new PR using OtherParsDefaults in the works.

@mohawk2
Copy link
Copy Markdown
Member

mohawk2 commented May 8, 2026

@duffee You'll also want to think about using =CALC(...) for the dim sizes. Probably go with OtherPars int lag => hin, default lag to -1, then h=CALC($SIZE(hin) < 0 ? $SIZE(t)-1 : $SIZE(hin))

Or, skip the middle-man, and have OtherPars int lag, default as above, and h=CALC($COMP(lag) < 0 ? $SIZE(t)-1 : $COMP(lag))

mohawk2 added a commit that referenced this pull request May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants