Skip to content
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.

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

Potential issues in scico.solver #486

Open
bwohlberg opened this issue Dec 15, 2023 · 0 comments
Open

Potential issues in scico.solver #486

bwohlberg opened this issue Dec 15, 2023 · 0 comments
Labels
bug Something isn't working discussion required Some discussion necessary to decide how to address this issue

Comments

@bwohlberg
Copy link
Collaborator

The comment on this line in scico.solver.minimize

x0 = x0.ravel() # if x0 is a BlockArray it will become a jax array here

is incorrect. This should be edited, and it should be confirmed that the functions does work for BlockArray input.

It's not clear whether the use of nonlocal and the "with side effects" is really necessary

scico/scico/solver.py

Lines 233 to 250 in 4eb21c1

def fun(x0):
nonlocal res # To use the external res and update side effect
res = spopt.minimize(
min_func,
x0=x0,
args=args,
jac=jac,
method=method,
options=options,
) # Returns OptimizeResult with x0 as ndarray
return res.x.astype(x0_dtype)
# HCB call with side effects to get the OptimizeResult on the same device it was called
res.x = hcb.call(
fun,
arg=x0,
result_shape=x0, # From Jax-docs: This can be an object that has .shape and .dtype attributes
)

It would be worth confirming this or simplifying the code.

@bwohlberg bwohlberg added bug Something isn't working discussion required Some discussion necessary to decide how to address this issue labels Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion required Some discussion necessary to decide how to address this issue
Projects
None yet
Development

No branches or pull requests

1 participant