Skip to content

cleanup#450

Open
FBartos wants to merge 1 commit into
jasp-stats:masterfrom
FBartos:refactor
Open

cleanup#450
FBartos wants to merge 1 commit into
jasp-stats:masterfrom
FBartos:refactor

Conversation

@FBartos

@FBartos FBartos commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Reorganized the R backend into smaller, purpose-specific files and consolidated duplicated helpers without changing analysis behavior or output.

Reorganized the R backend into smaller, purpose-specific files and consolidated duplicated helpers without changing analysis behavior or output.
@FBartos FBartos requested a review from juliuspfadt July 10, 2026 08:52
@juliuspfadt juliuspfadt requested a review from Copilot July 10, 2026 13:53
@juliuspfadt

Copy link
Copy Markdown
Contributor

build builder bot build for glory

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@juliuspfadt

Copy link
Copy Markdown
Contributor

I was going through the jasp files and found this. no idea if that is because of your PR:
error.jasp.zip
Screenshot 2026-07-10 at 16 46 59

@juliuspfadt juliuspfadt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. I am a bit lost as to how to review this, I checked the .jasp files and did some minor clicking around. I would still have some Ai review this, other than that it seems fine.

@juliuspfadt

Copy link
Copy Markdown
Contributor

Review summary

Verified the "no behavior change" claim both empirically (ran the test suite, root-caused the one failure) and structurally (diffed every one of the ~800 top-level functions' old body vs. new body across all 8 file families, via parallel review).

Test results: agentTestAll() — 289 pass, 2 fail, 10 skip. Both failures are in test-other-Meta-Regression - dat.bangertdrowns2004.R (analysis 4, heterogeneity-coefficient table) and are not caused by this PR — confirmed by loading the pre-refactor code (base commit eda15c5) directly and rerunning the same test, which produces the identical "new" numbers rather than the stale hardcoded expectations. That table already carries skip_on_os("linux") # numerical precision issues; it's a near-singular scale-model fit (SE ≈ 71–88k) sensitive to environment/BLAS drift, unrelated to this refactor.

Behavior-preservation audit: All 8 file families (bayesianmetaanalysisdeprecated, classicalmetaanalysis, classicalmetaanalysismultilevelmultivariate, forestplot, robustbayesianmetaanalysis, funnelplot/effectsizecomputation/selectionmodels, predictionperformance+misc) checked out clean — no dropped functions, no duplicated definitions anywhere in the repo, no style regressions. The genuine consolidations (.maShowMetaforRCode gaining a makeCallText param, .mammVarianceCovarianceMatrixDetails(), the new metaanalysis-export-helpers.R/metaanalysis-r-code-helpers.R) are all correctly wired to their call sites.

Minor, non-blocking nits:

  • R/classicalmetaanalysis-plots.R:56-68 and R/classicalmetaanalysis-model-summary-tables.R:307-321 — leftover runs of 15-16 blank lines from cutting functions out of the monolith. Cosmetic only.
  • The PR description says "consolidated duplicated helpers," but for prediction-performance specifically the one real duplicate (the metamisc::valmeta() arg-construction block in classical vs. Bayesian fit functions) wasn't actually merged — still copy-pasted in both files post-move. Not a bug, just a missed opportunity / slightly imprecise description.
  • Pre-existing, unrelated to this PR: library(metaSEM) in R/metaanalyticsem.R:25 violates the repo's no-library() convention — worth a future cleanup.

Recommendation: Approve. Well-executed, verified behavior-preserving refactor. Suggest one final clean agentTestAll() run before merge (a vdiffr snapshot blip showed up in one isolated single-analysis run but didn't reproduce in the full suite — likely test-run-order noise, not a real regression).

@FBartos

FBartos commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Yeah, some of the meta-sem visualizations do not work (not a new issue) and shall be fixed in a later separate PR.

The goal of this PR was just rearrange the files because they became too long which is inefficient for AI maintenance.

@juliuspfadt juliuspfadt self-requested a review July 11, 2026 07:04
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.

3 participants