Skip to content

Merge GraftingConfig into PreconditionerConfig #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Merge GraftingConfig into PreconditionerConfig #242

wants to merge 1 commit into from

Conversation

tsunghsienlee
Copy link
Contributor

@tsunghsienlee tsunghsienlee commented Aug 28, 2025 •
edited
Loading

Summary: Grafting is also instantiating a class whose base class is PreconditionerList so it makes sense to merge GraftingConfig into PreconditionerConfig. Moreover, this enriches the choices of both grafting and preconditioners.

Similar idea is also proposed in #227.

Differential Revision: D81237972

Co-authored-by: @Vishal-sys-code

@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 Aug 28, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D81237972

tsunghsienlee added a commit to tsunghsienlee/optimizers that referenced this pull request Aug 28, 2025
@tsunghsienlee @facebook-github-bot
Merge GraftingConfig into PreconditionerConfig (facebookresearch#242
ef22dd3 
)

Summary:

Grafting is also instantiating a class whose base class is `PreconditionerList` so it makes sense to merge `GraftingConfig` into `PreconditionerConfig`. Moreover, this enriches the choices of both grafting and preconditioners.

Differential Revision: D81237972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D81237972

tsunghsienlee added a commit to tsunghsienlee/optimizers that referenced this pull request Aug 28, 2025
@tsunghsienlee @facebook-github-bot
Merge GraftingConfig into PreconditionerConfig (facebookresearch#242
db41e3c 
)

Summary:

Grafting is also instantiating a class whose base class is `PreconditionerList` so it makes sense to merge `GraftingConfig` into `PreconditionerConfig`. Moreover, this enriches the choices of both grafting and preconditioners.

Differential Revision: D81237972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D81237972

@tsunghsienlee @facebook-github-bot
Merge GraftingConfig into PreconditionerConfig (facebookresearch#242
d290e42 
)

Summary:

Grafting is also instantiating a class whose base class is `PreconditionerList` so it makes sense to merge `GraftingConfig` into `PreconditionerConfig`. Moreover, this enriches the choices of both grafting and preconditioners.

Differential Revision: D81237972
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D81237972

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 80562ff.

@tsunghsienlee tsunghsienlee mentioned this pull request Aug 28, 2025
Closed
@Vishal-sys-code
Copy link

Thanks @tsunghsienlee for following through on this! Totally understand why this PR was closed in favor of the unified design in #242, it’s much cleaner.

Sorry for my late reply earlier (time zone difference on my side). Really appreciate being credited as co-author there. Glad I could contribute to the direction, and I’ll be happy to help review or test future grafting/preconditioner work.

@tsunghsienlee tsunghsienlee mentioned this pull request Aug 29, 2025
Closed
tsunghsienlee added a commit to tsunghsienlee/optimizers that referenced this pull request Aug 29, 2025
@tsunghsienlee @facebook-github-bot
Update README.md according to GraftingConfig merge (facebookresea…
aac6ba5 
…rch#243)

Summary:

Due to facebookresearch#242, `README.md` needs updated accordingly.

Differential Revision: D81278578
facebook-github-bot pushed a commit that referenced this pull request Sep 2, 2025
@tsunghsienlee @facebook-github-bot
Update README.md according to GraftingConfig merge (#243)
b84794c 
Summary:
Pull Request resolved: #243

Due to #242, `README.md` needs updated accordingly.

Reviewed By: wz337

Differential Revision: D81278578

fbshipit-source-id: 27db512c63481f30f4e4b0daad38b8a7518a9ae3
facebook-github-bot pushed a commit that referenced this pull request Sep 12, 2025
@tsunghsienlee @facebook-github-bot
Fix typing comment on grafting_config hyperparameter
2be5da9 
Summary: After the merge of `GraftingConfig` and `PreconditionerConfig` in #242, this comment should be correct as `PreconditionerConfig`.

Reviewed By: runame

Differential Revision: D82325249

fbshipit-source-id: bb957474e777eaac874e54e3cbfbe9fe163042ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tsunghsienlee @facebook-github-bot @Vishal-sys-code