Skip to content

Fix L-BFGS options#2080

Open
vl-dud wants to merge 1 commit intolululxvi:masterfrom
vl-dud:fix-lbfgs-options
Open

Fix L-BFGS options#2080
vl-dud wants to merge 1 commit intolululxvi:masterfrom
vl-dud:fix-lbfgs-options

Conversation

@vl-dud
Copy link
Copy Markdown
Contributor

@vl-dud vl-dud commented Mar 31, 2026

This PR fixes dde.optimizers.set_LBFGS_options so that it correctly updates iter_per_step and fun_per_step used by L-BFGS in PyTorch/Paddle backends.

Before PR:

dde.optimizers.set_LBFGS_options(maxiter=123, maxfun=456)
print(dde.optimizers.LBFGS_options)
# {'maxcor': 100, 'ftol': 0, 'gtol': 1e-08, 'maxiter': 123, 'maxfun': 456, 'maxls': 50, 'iter_per_step': 1000, 'fun_per_step': 1250}

After PR:

dde.optimizers.set_LBFGS_options(maxiter=123, maxfun=456)
print(dde.optimizers.LBFGS_options)
# {'maxcor': 100, 'ftol': 0, 'gtol': 1e-08, 'maxiter': 123, 'maxfun': 456, 'maxls': 50, 'iter_per_step': 123, 'fun_per_step': 456}

LBFGS_options["maxfun"] = maxfun if maxfun is not None else int(maxiter * 1.25)
LBFGS_options["maxls"] = maxls

# Backend-dependent options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please verify my understanding: set_hvd_opt_options sets the LFBGS options along with doing the backend-specific operations, but in reality set_LFBGS_options should handle this itself, causing the bug that when you call set_LGBGS_options by itself, it wont update iter_per_step and fun_per_step.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

set_LBFGS_options is expected to update all L-BFGS-related parameters, including iter_per_step and fun_per_step used in the Paddle and PyTorch backends. However, the current implementation does not update them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok. This looks good.

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.

2 participants