Basis optimization CLI #910

pfebrer · 2025-04-14T23:06:42Z

Here is the PR with the basis optimization CLI.

Probably I should add something to the documentation.

@zerothi could you test if it works fine for you? You just need to have the inputs for a calculation in a directory and then run:

stoolbox basis optim --geometry file.fdf

The only constraint is that the fdf file can't be named RUN.fdf.

EDIT: If you want to optimize with BADS (the default optimizer) you just need to install pybads:

pip install pybads

src/sisl_toolbox/siesta/minimizer/_minimize.py

+            # This might be a problem if the arguments are not pickleable. E.g. the minimizer function
+            # is associated with the minimizer object, which might contain siles.
+            try:
+                opt = bads.optimize()


zerothi · 2025-05-13T11:42:49Z

src/sisl_toolbox/siesta/minimizer/_minimize.py

+            # is associated with the minimizer object, which might contain siles.
+            try:
+                opt = bads.optimize()
+            except TypeError:


is there a reason for try ... pass? Don't we want it to scream at the user if it fails?

This is explained in the comment above, basically their final saving or whatever (I don't remember exactly what it was) uses pickle and it can fail, but I guess we didn't need it because we do our own saving of the optimization.

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+        for pre, f in [(">", stdout), ("2>", stderr)]:
+            try:
+                pipe += f"{pre} {f.name}"
+            except Exception:


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+                cmd[:-2],
+                cwd=self.path,
+                encoding="utf-8",
+                stdin=open(self.fdf, "r"),


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+                # Retrieve delta-x for the jacobian for this
+                eps = minimize.normalize("delta", with_offset=False)
+
+                result = minimize.run(


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+                    **optimizer_kwargs,
+                )
+            elif optimizer == "swarms":
+                result = minimize.run(**optimizer_kwargs)


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+            elif optimizer == "swarms":
+                result = minimize.run(**optimizer_kwargs)
+            else:
+                result = minimize.run(


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

src/sisl_toolbox/siesta/atom/_register_cli.py

+    title = "Plotting facility for atom output (run in the atom output directory)"
+    if is_sub:
+        global _script
+        _script = f"{_script} atom-plot"


src/sisl_toolbox/siesta/minimizer/__init__.py

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

zerothi

I'll play with it at its current state, but some comments here and there would be beneficial to be cleared out!

src/sisl_toolbox/siesta/minimizer/_minimize.py

zerothi · 2025-05-13T11:39:31Z

src/sisl_toolbox/siesta/minimizer/_minimize.py

-        self.data.hash.append(current_hash)
+        metric = self(variables, *args)
+
+        if metric is not None:


when would metric be none? Would it even be viable?

Shouldn't something break if the metric is none?

I think None meant that it has not been able to compute the metric, e.g. the calculation has failed. It shouldn't break because the optimizer can set variable values that result in a failed calculation.

So, the way the old thing was working is that if that metric broke, it had to return some too high number. Otherwise some of the minimizations couldn't cope, i.e. it calculates something, expects a metric to be there, but doesn't find one, how should it then select an appropriate next point. SWARM, and others might be able too, but those that have deterministic decisions won't, right?

So, I guess a metric that fails, should scream, did you encounter this?

I think I recall having tested with all the minimizers and they all handled a None, but I can't say 100%

zerothi · 2025-05-13T11:40:30Z

src/sisl_toolbox/siesta/minimizer/_minimize.py

+        bounds = self.normalize_bounds()
+        bounds = np.array(bounds)
+
+        bounds[bounds == 0] = 0.5


Why is this?

What happens when one has a bounds [0, 0.2] then it would be changed to [0.5, 0.2]?

Yes, I guess, but who has 0.2 as the maximum cutoff for the basis? I agree that this is not general, And I don't remember what happened if one of the bounds was 0, maybe something nasty 😅

but if you want 0.5 to be the minimum, then we should have bounds[bounds < 0.5] = 0.5, right?

Otherwise we should select a sensible minimum, and let users control everything.

Yes clearly something should be done if there is someone who wants to use this for something other than basis optimization.

Just read through my old yaml conf, we have to remove this, because there are soft-confinement things that uses negative bounds, so you would have to know which kind of boundary it attaches to...

This check was only done for the bads, but in principle, it should be there for all, so I'll remove it...

zerothi · 2025-05-13T11:41:23Z

src/sisl_toolbox/siesta/minimizer/_minimize.py

+    It uses the pybads package to perform the minimization.
+    """
+
+    def run(self, options={}, get_hard_bounds: Callable = lambda x: x, **kwargs):


I would rename get_hard_bounds to func_conv_bounds_to_hard or something more descriptive.

Whatever you prefer

zerothi · 2025-05-13T11:42:49Z

src/sisl_toolbox/siesta/minimizer/_minimize.py

+            # is associated with the minimizer object, which might contain siles.
+            try:
+                opt = bads.optimize()
+            except TypeError:


is there a reason for try ... pass? Don't we want it to scream at the user if it fails?

zerothi · 2025-05-13T11:48:56Z

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

@@ -0,0 +1,1031 @@
+# This Source Code Form is subject to the terms of the Mozilla Public


how much of this file is duplicating the yaml conf file? Seems to have some overlap?

I don't know how to answer this 😅

zerothi · 2025-05-13T12:10:41Z

Seems like we should move the basis optim writer to the _yaml_reader and rename it.

Signed-off-by: Nick Papior <[email protected]>

src/sisl_toolbox/siesta/minimizer/_register_cli.py

+    get_argparse_parser(
+        write_basis_to_yaml, name="build", subp=subp, parser_kwargs=parser_kwargs
+    )
+    parser_kwargs["aliases"] = ("optim",)


Signed-off-by: Nick Papior <[email protected]>

zerothi · 2025-06-19T20:13:41Z

@pfebrer could you please review my comments, then we should get it in asap (note I forced pushed a rebase!)

pfebrer · 2025-06-19T20:16:14Z

Hey, I have answered them, sorry I think I was too busy when I saw the review

codecov · 2025-06-19T20:20:39Z

Codecov Report

Attention: Patch coverage is 22.09302% with 67 lines in your changes missing coverage. Please review.

Project coverage is 86.82%. Comparing base (58d60d8) to head (ec336e9).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sisl/_lib/_argparse.py	22.61%	65 Missing ⚠️
src/sisl/_core/periodictable.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #910      +/-   ##
==========================================
- Coverage   86.92%   86.82%   -0.10%     
==========================================
  Files         412      412              
  Lines       54332    54413      +81     
==========================================
+ Hits        47227    47244      +17     
- Misses       7105     7169      +64

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pfebrer · 2025-06-19T20:31:32Z

src/sisl_toolbox/siesta/minimizer/_register_cli.py

    get_argparse_parser(
-        write_basis_to_yaml, name="build", subp=subp, parser_kwargs=parser_kwargs
+        optimize_basis, name="optimize", subp=subp, parser_kwargs=parser_kwargs


You need to update the documentation too then, at docs/toolbox/basis/basis.rst

Or is optim also valid?

both are valid, but yes, I should have done that (will do!)

zerothi · 2025-06-20T07:18:04Z

I have some questions, the charge-confinement scheme is not only targeted polarization orbitals. AFAIK, it's use is for non-populated orbitals, and so could be either regular empty orbitals or something else.

I will also add the soft-confinement to play with. Do you have any comments on this before I make changes?

zerothi · 2025-06-20T12:04:52Z

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+
+    Appropiate optimization bounds are set for each parameter.\n\n
+
+    At each step, all corresponding parameters are optimized simultaneously.\n\n


could you clarify for me here, I tried reading it twice, but I don't understand the flow.

It first reads, it will optimize them sequentially. Then it says that all parameters are optimized simultaneously?
What am I not understanding?

The simultaneously means that for example when optimizing the first zeta shell, all first zeta cutoff radii (for all atoms) are optimized at the same time. And same for the polarization orbitals

Ok, but not that they would all cycle the same numbers, right?

Added needed dependency for pyyaml. While strictly not necessary, I think this is an ok thing. It is very much used elsewhere, and we might require it for other things. Made an alias for stoolbox basis optimize|optim. Fixed a thing for return numpy arrays in periodictable. The new CLI registry requires handling of union types, this will fix it by converting unions to str, regardless of what it actually is. Added information when basis information is requested but the entries won't be there due to too small numbers. E.g. soft confinement with V0 = 0 will effectively not do anything. Same for charge confinement for abs(Z) = 0. Added soft-confinement. Made charge-confinement available for all orbitals with 0 charge. Soft-confinement are added to all orbitals, which likely is overkill. It should probably only be done for narrow orbitals. Well... When *guessing* the orbitals I found some errors, at least for gold, it simply omitted the p orbitals, so now there is a warning, and then it will continue silently. It's better to rely on user-defined basis-information. Enabled reading psml files from SIESTA_PS_PATH (not psf!). Signed-off-by: Nick Papior <[email protected]>

zerothi · 2025-06-20T13:28:22Z

@pfebrer could you have a look at the latest commit I made, I added soft-confinement, and changed the logic of the charge-confinement (now for q=0 orbitals + polarization orbitals).

Plus some other minor details.

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+    if basis_size != "atoms":
+        try:
+            n_zetas = int(basis_size[0])
+        except:


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+
+    # Loop through atoms and generate a basis config for each, adding it to config_dict.
+    for atom in atoms:
+        table_row = PT.Z_row(abs(atom.Z))


src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+    # Loop through atoms and generate a basis config for each, adding it to config_dict.
+    for atom in atoms:
+        table_row = PT.Z_row(abs(atom.Z))
+        atom_block = PT.Z_block(abs(atom.Z))


pfebrer · 2025-06-20T14:20:43Z

Regarding the charge confinement and soft-confinement, the basis optimization follows a workflow that Federico determined was general and good enough to optimize the basis in "any" system. So it was not meant to be very flexible in what you can optimize, but that was more a feature than a bug I would say. If you want to change something about the workflow, I would talk to Federico

pfebrer · 2025-06-20T14:41:31Z

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+        warn(
+            "The orbitals are pre-filled, but likely they are not correct. "
+            "Please check the basis before doing excessive optimizations!"
+        )


Why likely they are not correct? 😅

I tested this on gold, and it gave me s, d and f shells, no p... And I wouldn't like to run through them all, so better be safe than sorry ;)

pfebrer · 2025-06-20T14:52:16Z

src/sisl_toolbox/siesta/minimizer/basis_optimization.py

+        If true, it will optimize, the potential (:math:`V_0`), and the inner
+        radius :math:`r_i`.
+
+        For multiple arguments, each can be fine-tuned on/off.


Does this work from the CLI? If so, how?

Doesn't it make the CLI harder to understand?
--optimize-soft-conf false will optimize soft confinement.

For the charge confinement, are you sure optimizing the three parameters at the same time will result in a good optimization/ is worth it? I also don't understand to which orbitals it applies, Federico said that the charge optimization should only be done for polarization orbitals. So again make sure to talk to Federico because I am not sure that this added flexibility is good.

It works by --opt true|true,false,false e.g.
If you say false, it should not run it, but perhaps I forgot to do the proper conversion here... I just did bool on arguments, which when I think about it is wrong. ;)

I'll talk with fede.

github-advanced-security bot found potential problems Apr 14, 2025

View reviewed changes

github-advanced-security bot found potential problems Apr 15, 2025

View reviewed changes

zerothi requested changes May 13, 2025

View reviewed changes

pfebrer and others added 7 commits June 19, 2025 22:03

Basis optimization CLI

1daf5e8

Long description for the parser

83a6aab

Document that enthalpy is minimized

5e99196

Removed unused imports

fd4469e

Added documentation

be064dd

typos

cb85646

added alias for optim

65de7d6

Signed-off-by: Nick Papior <[email protected]>

zerothi force-pushed the basis_optim branch from 4967875 to 65de7d6 Compare June 19, 2025 20:03

github-advanced-security bot found potential problems Jun 19, 2025

View reviewed changes

added pyyaml as dependency

d900425

Signed-off-by: Nick Papior <[email protected]>

pfebrer commented Jun 19, 2025

View reviewed changes

zerothi reviewed Jun 20, 2025

View reviewed changes

github-advanced-security bot found potential problems Jun 20, 2025

View reviewed changes

pfebrer commented Jun 20, 2025

View reviewed changes

		@@ -0,0 +1,1031 @@
		# This Source Code Form is subject to the terms of the Mozilla Public


		Appropiate optimization bounds are set for each parameter.\n\n

		At each step, all corresponding parameters are optimized simultaneously.\n\n

Uh oh!

Basis optimization CLI #910

Are you sure you want to change the base?

Basis optimization CLI #910

Uh oh!

Conversation

pfebrer commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check notice

Check notice

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check notice

Check warning

Uh oh!

Check warning

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Check notice

Uh oh!

Uh oh!

Uh oh!

zerothi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfebrer Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerothi commented May 13, 2025

Uh oh!

Check failure

Uh oh!

zerothi commented Jun 19, 2025

Uh oh!

pfebrer commented Jun 19, 2025

Uh oh!

codecov bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

pfebrer commented Apr 14, 2025 •

edited

Loading

pfebrer Jun 19, 2025 •

edited

Loading

codecov bot commented Jun 19, 2025 •

edited

Loading

pfebrer Jun 20, 2025 •

edited

Loading