Skip to content

Refactor/perf batch 3#471

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

Refactor/perf batch 3#471
timcadman merged 19 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

  • Signature takes the object name (string) instead of the resolved vector — e.g. meanDS(x) not meanDS(xvect)
  • .loadServersideObject(x) resolves the object server-side
  • .checkClass(obj, obj_name, permitted_classes = ...) enforces type (e.g. c("numeric","integer"))
  • Result list gains a class = class(x.val) field where class needs to be checked for consistency across nodes so that the client can do its consistency check from data already returned (no extra round-trip)
  • ValidityMessage / studysideMessage removed

Tests

  • New test-smk-meanSdGpDS.R
  • Updated smk tests for cor, corTest, cov, kurtosis1/2, mean, quantileMean, skewness1/2, var
  • Updated test-disc- and test-perf- for meanDS/varDS

@timcadman timcadman marked this pull request as ready for review April 14, 2026 11:51
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@timcadman timcadman marked this pull request as draft April 14, 2026 12:00
@timcadman timcadman force-pushed the refactor/perf-batch-3 branch from cce265c to 1ff323f Compare April 14, 2026 15:07
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@timcadman timcadman marked this pull request as ready for review April 14, 2026 15:07
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Server-side check preserves pre-refactor contract that was dropped
when client-side validation moved to server. Redocument levelsDS.
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@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.

Does "default.datashield.privacyControlLevel need to be set to "permissive"?

@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.

  • Not sure why "default.datashield.privacyControlLevel" is now set to "permissive", not "banana" as usual default.
  • Suspect documentation needs to be regenerated after PR acceptance.

@StuartWheater StuartWheater self-requested a review July 14, 2026 09:37

@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.

Updates appear consistent

@timcadman timcadman merged commit 42113c4 into v7.0-dev Jul 14, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants