Skip to content

Refactor/perf batch 3#662

Merged
timcadman merged 42 commits into
v7.0-devfrom
refactor/perf-batch-3
Jul 16, 2026
Merged

Refactor/perf batch 3#662
timcadman merged 42 commits into
v7.0-devfrom
refactor/perf-batch-3

Conversation

@timcadman

@timcadman timcadman commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

What's changed

  • remove isDefined() and checkClass() and replace with serverside checks
  • Class checking replaced by .checkClassConsistency() run on the class field already embedded in the function's normal result — no extra round-trip
  • ValidityMessage handling removed
  • Adapted to new list-based server returns where class is also returned (dimDS/lengthDS/numNaDS)
  • utils.R: .checkClassConsistency() generalised with field + object_name args (handles classes returned under different element names, e.g. class.x, class.index)

Tests

  • New test-perf- for cor, corTest, cov, kurtosis, meanSdGp, quantileMean, skewness, summary, var
  • Smoke tests: added wrong-class checks; updated mean/var/quantileMean expectations
  • Test expectations in tests/testthat/smk_expt-results/ regenerated because ds.mean no longer returns validity message
  • Removed orphaned test-arg- checks (moved to smoke); regenerated ds.mean fixtures

Other

  • Updated dsBase_7.0.0-permissive.tar.gz for CI

@timcadman timcadman changed the base branch from master to v7.0-dev April 13, 2026 12:51
@StuartWheater StuartWheater added this to the v7.0.0 milestone Jun 8, 2026
@timcadman timcadman marked this pull request as ready for review June 15, 2026 09:45

@StuartWheater StuartWheater left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the name of tar is to change name (added ".9000") the file "armadillo_azure-pipelines.yml" will need also changed. [Approx line 239]

@timcadman

Copy link
Copy Markdown
Contributor Author

If the name of tar is to change name (added ".9000") the file "armadillo_azure-pipelines.yml" will need also changed. [Approx line 239]

Agh that wasn't intentional - will revert!

1 similar comment
@timcadman

Copy link
Copy Markdown
Contributor Author

If the name of tar is to change name (added ".9000") the file "armadillo_azure-pipelines.yml" will need also changed. [Approx line 239]

Agh that wasn't intentional - will revert!

@StuartWheater StuartWheater self-requested a review July 16, 2026 12:27

@StuartWheater StuartWheater left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Replacement 'dsBase-v7.0.tar', and associated 'dsBase-v7.0-permissive.tar' fixes issues.

@timcadman timcadman merged commit 0e88c63 into v7.0-dev Jul 16, 2026
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants