Skip to content

develop the acro_summarise() function - #65

Open
mahaalbashir wants to merge 10 commits into
mainfrom
64-developing-the-acro_summarise-function
Open

develop the acro_summarise() function#65
mahaalbashir wants to merge 10 commits into
mainfrom
64-developing-the-acro_summarise-function

Conversation

@mahaalbashir

Copy link
Copy Markdown
Collaborator
  1. The acro_summarise() function uses the acro pivot_table function to create the output and do the risk assessments.
  2. It works with piping.

@mahaalbashir mahaalbashir linked an issue Jul 27, 2026 that may be closed by this pull request
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f942c8d) to head (7c63c35).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          198       274   +76     
=========================================
+ Hits           198       274   +76     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mahaalbashir
mahaalbashir requested a review from jim-smith July 27, 2026 21:33
Comment thread tests/testthat/test-acro_summarise.R Outdated
mahaalbashir and others added 2 commits July 28, 2026 11:04
Co-authored-by: Jim-smith <[email protected]>
Signed-off-by: mahaalbashir <[email protected]>
Signed-off-by: mahaalbashir <[email protected]>
@mahaalbashir
mahaalbashir requested a review from jim-smith July 28, 2026 10:50

@jim-smith jim-smith Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All the tests that she acro_summarise behave exactly like R summarise are great.
Is there a way of checking the risk assessments produced and fed back to the researcher?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jim-smith I added three tests:

  1. Check the summary when the status is Pass
  2. Check the summary when the status is Fail
  3. Check the summary when suppression is enabled (status review)

Do you think of any other test cases that we can add?

@jim-smith

Copy link
Copy Markdown
Collaborator

@mahaalbashir
just thinking about how we let people know about these - and make the help etc available when researchers are (virtually) in TREs without internet access.

  1. Do we need manually provide the vignette's or are these automatically created from the docstrings?
  2. Should there be more updates/documents/example notebooks showing or discussing how to use acro from within the tidy verse?

@mahaalbashir

Copy link
Copy Markdown
Collaborator Author

@jim-smith could you please take a look at the tests I have added to test that suppression is actually applied to the correct cells

output <- py_results$get_index(as.integer(0))

# Verify summary string matches expected SDC risk assessment
correct_summary <- "fail; threshold: 1 cells may need suppressing; p-ratio: 1 cells may need suppressing; nk-rule: 1 cells may need suppressing; "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
correct_summary <- "fail; threshold: 1 cells may need suppressing; p-ratio: 1 cells may need suppressing; nk-rule: 1 cells may need suppressing; "
correct_summary <- "fail; threshold: 5 cells may need suppressing; p-ratio: 1 cells may need suppressing; nk-rule: 1 cells may need suppressing; "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should be 4 cells failing min threshold if you don't have totals

output <- py_results$get_index(as.integer(0))

# Verify summary string matches expected SDC risk assessment
correct_summary <- "fail; threshold: 1 cells may need suppressing; p-ratio: 1 cells may need suppressing; nk-rule: 1 cells may need suppressing; "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

alternative would to check that the status is fail,
the correct summary string is going to depend on whether it is using old or new style formatting

output <- py_results$get_index(as.integer(0))

# Verify summary string matches expected SDC risk assessment
correct_summary <- "review; threshold: 1 cells suppressed; p-ratio: 1 cells suppressed; nk-rule: 1 cells suppressed; "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In light of imminent changes, more robust to check that the status is review and an exception has been added: "Suppression automatically applied where needed"

expect_equal(as.character(output$summary), correct_summary)
})

test_that("acro_summarise() suppression for the threshold matches R summarise() on the dataset filtered for the threshold", {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jim-smith, what about this test and the tests below? Do you think it is doing what we discussed last time about checking if acro is actually suppressing the correct cells by filtering the data to remove disclosive cells, then doing the analysis and comparing it to the acro suppressed version of the table?

expect_equal(as.character(output$summary), correct_summary)
})

test_that("acro_summarise() suppression for the threshold matches R summarise() on the dataset filtered for the threshold", {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mahaalbashir
if you replace sum with points you get this pivot table which suggests you're only going ot be left with one cell:

Image

maybe try different independent variables?

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.

Developing the acro_summarise() function

2 participants