Skip to content

optimizer as an argument for optimize #365

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

Merged
bernstei merged 6 commits into from
Jul 22, 2026
Merged

optimizer as an argument for optimize #365

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf14f71
Update optimize.py
jungsdao Sep 13, 2025
0169a99
fix conflict
Jul 21, 2026
3e6971c
differentiate precon/normal optimizer
Jul 21, 2026
3bae7af
change docstring
Jul 21, 2026
82bc9c2
apply patch
Jul 22, 2026
48cbcad
change buildcell
Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions wfl/generate/optimize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def _new_log(self, forces=None):
def _run_autopara_wrappable(atoms, calculator, fmax=1.0e-3, smax=None, steps=1000, pressure=None,
stress_mask=None, keep_symmetry=True, traj_step_interval=1, traj_subselect=None,
skip_failures=True, results_prefix='last_op__optimize_', verbose=False, update_config_type="append",
rng=None, _autopara_per_item_info=None,
optimizer=PreconLBFGS, rng=None, _autopara_per_item_info=None,
**opt_kwargs):
"""runs a structure optimization. By default calculator properties will be stored in keys
prefixed with "last_op__optimize_", which may be overwritten by next operation.
Expand Down Expand Up @@ -73,6 +73,8 @@ def _run_autopara_wrappable(atoms, calculator, fmax=1.0e-3, smax=None, steps=100
optimisation logs are not printed unless this is True
update_config_type: ["append" | "overwrite" | False], default "append"
whether/how to add at.info['optimize_config_type'] to at.info['config_type']
optimizer : ASE optimizer
optimizer to use, default LBFGSPrecon
Comment thread
jungsdao marked this conversation as resolved.
Outdated
opt_kwargs
keyword arguments for PreconLBFGS
rng: numpy.random.Generator, default None
Expand All @@ -93,7 +95,7 @@ def _run_autopara_wrappable(atoms, calculator, fmax=1.0e-3, smax=None, steps=100

calculator = construct_calculator_picklesafe(calculator)

if smax is None:
if "precon" in optimizer.__module__.split(".") and smax is None:

@bernstei bernstei Jul 21, 2026 •
edited
Loading

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.

Would it be safer to do isinstance(optimizer, PreconLBFGS)?

@jungsdao jungsdao Jul 21, 2026

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.

It seems isinstance is not good way to check it.

optimizer = PreconLBFGS
isinstance(optimizer, PreconLBFGS)
## False

Also there's PreconFIRE. I don't have better idea than this but if you have better solution, please let me know I'll adapt it.

@bernstei bernstei Jul 21, 2026

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.

You're right, isinstance is wrong. Maybe optimizer.__name__ in ["PreconLBFGS", "PreconFIRE"]? Really it would be better to do this by trying to pass smax to the dynamics and falling back if it fails. Let me try to write something like that.

@bernstei bernstei Jul 21, 2026

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.

Try this diff file
opt_smax_diff.txt

@bernstei bernstei Jul 21, 2026

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.

Could also write it with nested try/except, but I think that ends up uglier

@bernstei bernstei Jul 21, 2026

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.

This is the try/catch version
opt_smax_diff_trycatch.txt

Neither is well tested

@jungsdao jungsdao Jul 22, 2026

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.

It seems to work for both precon and non-precon optimizers though it's a bit longer.
There seems to be a test failures which is not related to this PR.

@bernstei bernstei Jul 22, 2026

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.

Looks like for some reason gap_fit doesn't exist in the CI. I'll merge this PR, and then try to figure out why that's happening.

smax = fmax

if keep_symmetry:
Expand Down Expand Up @@ -134,7 +136,7 @@ def _run_autopara_wrappable(atoms, calculator, fmax=1.0e-3, smax=None, steps=100
else:
wrapped_at = at

opt = PreconLBFGS(wrapped_at, **opt_kwargs_to_use)
opt = optimizer(wrapped_at, **opt_kwargs_to_use)

# default status, will be overwritten for first and last configs in traj
at.info['optimize_config_type'] = 'optimize_mid'
Expand Down Expand Up @@ -162,7 +164,10 @@ def process_step():
converged = False

try:
converged = opt.run(fmax=fmax, smax=smax, steps=steps)
if smax is None:
converged = opt.run(fmax=fmax, steps=steps)
else:
converged = opt.run(fmax=fmax, smax=smax, steps=steps)
except Exception as exc:
# label actual failed optimizations
# when this happens, the atomic config somehow ends up with a 6-vector stress, which can't be
Expand Down
Loading