Add get_noise_multiplier API for DP-wrapped models #38
Conversation
- Add public API to retrieve calculated noise multiplier - Maintain DPKerasConfig immutability - Add comprehensive unit tests - Update example with usage demonstration - Pre-calculate noise multiplier in make_private for efficiency Addresses user feedback about accessing auto-calculated noise multiplier for logging purposes.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
RamSaw
left a comment
RamSaw
left a comment
There was a problem hiding this comment.
Thank you very much for such a great contribution!
In general LGTM but left few minor comments.
In PR description you write that you update examples, but in PR changed files there are no changes to examples. Not saying that it is necessary, without it PR is also fine, but just doulbe checking in case you forgot to push a commit with these changes.
| # that updates the metrics variables for DP-SGD training. | ||
|
|
||
| # Pre-calculate noise multiplier if not provided | ||
| if params.noise_multiplier is None: |
There was a problem hiding this comment.
then we should remove
noise_multiplier = (
dp_params.noise_multiplier
if dp_params.noise_multiplier is not None
else dp_params.update_with_calibrated_noise_multiplier().noise_multiplier
)
in _noised_clipped_grads function
There was a problem hiding this comment.
Thanks! Since make_private guarantees that noise_multiplier is initialized, I went ahead and replaced the conditional logic with an assertion to keep things cleaner.
jax_privacy/keras/keras_api.py
Outdated
|
|
||
| # Otherwise, calculate it (this will use the cached value if already computed) | ||
| updated_params = dp_params.update_with_calibrated_noise_multiplier() | ||
| return updated_params.noise_multiplier |
There was a problem hiding this comment.
nit: add a new line in the end of the file
jax_privacy/keras/keras_api.py
Outdated
| dp_params = model._dp_params # pylint: disable=protected-access | ||
|
|
||
| # If noise_multiplier was provided, return it directly | ||
| if dp_params.noise_multiplier is not None: |
There was a problem hiding this comment.
we come here only if make_private was called, then it means dp_params must have noise multiplier, so we can just return here asserting that it is not None.
jax_privacy/keras/keras_api.py
Outdated
| if dp_params.noise_multiplier is not None: | ||
| return dp_params.noise_multiplier | ||
|
|
||
| # Otherwise, calculate it (this will use the cached value if already computed) |
There was a problem hiding this comment.
just fyi, there is no caching because it is not cached property
jax_privacy/keras/keras_api_test.py
Outdated
| model = keras_api.make_private(model, params) | ||
| retrieved_noise_mult = keras_api.get_noise_multiplier(model) | ||
|
|
||
| self.assertIsInstance(retrieved_noise_mult, float) |
There was a problem hiding this comment.
Good point — this check is probably redundant since get_noise_multiplier is already type-hinted to return a float.
|
Also, to merge you need to sign CLA. |
|
Thanks for the feedback, This is my first open source contribution, so I appreciate you taking the time to review it. I've pushed the fix. |
2 participants
Title
Add get_noise_multiplier(model) API to expose calculated noise multiplier (Fixes #26)
Description
Fixes #26 — When DPKerasConfig.noise_multiplier is None, the auto-calculated value was not accessible because the config is immutable. This update introduces a public API to retrieve the effective noise multiplier from a DP-wrapped Keras model.
What Changed
Why
Maintains DPKerasConfig immutability while giving users a simple, reliable way to log or inspect the effective noise multiplier for transparency and debugging.
Testing