Skip to content

patch bug in IAccountant len functions. Currently returns # of unique noise_multiplier values vs intended # of steps#829

Open
eddiestudies wants to merge 3 commits into
meta-pytorch:mainfrom
eddiestudies:eddie/patch_len_always_one
Open

patch bug in IAccountant len functions. Currently returns # of unique noise_multiplier values vs intended # of steps#829
eddiestudies wants to merge 3 commits into
meta-pytorch:mainfrom
eddiestudies:eddie/patch_len_always_one

Conversation

@eddiestudies

@eddiestudies eddiestudies commented Jun 22, 2026

Copy link
Copy Markdown

Types of changes

  • [ x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change if dependent on len() == 1)
  • Docs change / refactoring / dependency upgrade

Motivation and Context / Related issue

Accountants currently implement len with len(self.history). This means that len(accountant) is 1 or # of noise_multiplier changes vs the documented "Number of optimization steps taken so far" . This is because self.history is compact and the true step count is accumulated into self.history[:][2].

How Has This Been Tested (if it applies)

I added iterative tests over existing accountants. For GDP, noise_multiplier changes raise a ValueError so it is ommitted from the noise_multiplier test.
I moved the fix into the base class since the contract for history is defined there. This should avoid future accountants from running into the same bug.

Checklist

  • The documentation is up-to-date with the changes I made.
  • I have read the CONTRIBUTING document and completed the CLA (see CONTRIBUTING).
  • All tests passed, and additional code has been covered with new tests.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 22, 2026
@meta-codesync

meta-codesync Bot commented Jun 22, 2026

Copy link
Copy Markdown

This pull request has been imported. If you are a Meta employee, you can view this in D109374292. (Because this pull request was imported automatically, there will not be any future comments.)

@eddiestudies eddiestudies changed the title Add patch to len function. Currently returns len history items vs number of optimization steps taken so far patch bug in IAccountant len functions. Currently returns # of unique noise samplers vs intended # of steps Jun 23, 2026
@eddiestudies eddiestudies changed the title patch bug in IAccountant len functions. Currently returns # of unique noise samplers vs intended # of steps patch bug in IAccountant len functions. Currently returns # of unique noise_multiplier values vs intended # of steps Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant