Validate structural arguments (num_bands, truncated sizes) in accounting events#284
Validate structural arguments (num_bands, truncated sizes) in accounting events#284r1cksync wants to merge 1 commit into
Conversation
887593d to
b2c7d0c
Compare
num_bands flows into math.ceil(iterations / num_bands) in amplified_bandmf_event and truncated_amplified_bandmf_event but was never validated: num_bands=0 raised an opaque ZeroDivisionError and num_bands<0 raised a misleading "iterations=..." error. Likewise truncated_dpsgd_event forwarded num_examples / truncated_batch_size to TruncatedSubsampledGaussianDpEvent without validation, so negative sizes were silently accepted and produced a nonsensical event. Guard num_bands with _validate.positive and the truncated sizes with _validate.non_negative (matching the existing _validate_fixed_args convention), so invalid inputs fail fast with a clear, consistent message. Only previously-invalid inputs are rejected; valid inputs are unchanged. Adds parameterized regression tests.
b2c7d0c to
c21fa32
Compare
|
This PR has been idle for 7 days. Please provide an update or review. |
arung54
left a comment
There was a problem hiding this comment.
Thanks for the PR. No issues with the changes in logic, but a few comments to keep the style consistent. Will be happy to approve once they're addressed.
| truncated_batch_size=16, | ||
| ) | ||
|
|
||
| def test_amplified_bandmf_event_valid_num_bands_unchanged(self): |
There was a problem hiding this comment.
I think '_unchanged' here and in the below test names is unnecessary and if anything adds confusion.
| # as the continuous Gaussian with the same sigma. | ||
| self.assertAlmostEqual(eps_continuous, eps_discrete, places=4) | ||
|
|
||
| # -- Structural-argument validation ---------------------------------------- |
There was a problem hiding this comment.
I think none of the comments introduced in this CL are necessary, and would suggest removing them all.
From the test names I think it is pretty easy to infer what each test is supposed to do, and the logic can be inferred from reading the file being tested. We generally prefer not to include comments if they only explain something that can be inferred by someone with knowledge of Python.
Bug
Several
jax_privacy.accountingevent builders forward user-controlled structural arguments into divisions / sizing without validating them, so invalid inputs either crash opaquely or are silently accepted:num_bandsflows intomath.ceil(iterations / num_bands)inamplified_bandmf_eventandtruncated_amplified_bandmf_event;num_examples/truncated_batch_sizeflow intoTruncatedSubsampledGaussianDpEventintruncated_dpsgd_event. None were validated, while sibling builders (fixed_dpsgd_eventvia_validate_fixed_args) already validate their sizes.Fix
_validate.positive(num_bands=num_bands)in both BandMF builders (must be ≥ 1 to partition the data and to divide by)._validate.non_negative(num_examples=..., truncated_batch_size=...)intruncated_dpsgd_event, matching the_validate_fixed_argsconvention for dataset/batch sizes.Only previously-invalid inputs (non-positive
num_bands, negative sizes) are rejected — valid inputs are unchanged. This mirrors the already-merged #255 and the existing_validatepattern.Testing
python -m pytest tests/accounting_test.py— 36 passed. The new parameterized tests cover each newly-guarded argument × invalid value, plus positive regressions; they fail (8 failures + 2 ZeroDivisionError) against the pre-fix code and pass after.