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

Respect prob.bounds in all solver wrappers #463

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

missing-user
Copy link
Contributor

Only the constrained_solve overloads respected the problem bounds, the least_squares_solve overloads ignored them, unless passed with bounds=prob.bounds explicitly. This is unintuitive, since these functions are specifically overloads for LeastSquaresProblem, so I would expect it to use all the properties set on the object.

I suggest harmonizing the usage of constrained_solve and least_squares_solve to in both cases respect prob.bounds (as implemented in this PR), or at the very least raising a warning message to users, if prob.bounds were set, despite being ignored (I will adjust the PR if required).

@missing-user
Copy link
Contributor Author

I also replaced print with logger.info in two places that stood out to me.

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

Attention: Patch coverage is 60.86957% with 9 lines in your changes missing coverage. Please review.

Project coverage is 92.64%. Comparing base (d6bea2b) to head (adb4dec).

Files with missing lines Patch % Lines
src/simsopt/solve/serial.py 58.33% 5 Missing ⚠️
src/simsopt/_core/optimizable.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
- Coverage   92.66%   92.64%   -0.03%     
==========================================
  Files          78       78              
  Lines       14514    14529      +15     
==========================================
+ Hits        13450    13460      +10     
- Misses       1064     1069       +5     
Flag Coverage Δ
unittests 92.64% <60.86%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@missing-user
Copy link
Contributor Author

Replaced deprecated np.product with np.prod for numpy 2.0 support

@landreman
Copy link
Contributor

Looks good to me. I think the failing tests are unrelated to the changes in this PR. Let's see if we can fix those in master. Anyone have ideas how to resolve those?

@missing-user
Copy link
Contributor Author

Looks good to me. I think the failing tests are unrelated to the changes in this PR. Let's see if we can fix those in master. Anyone have ideas how to resolve those?

Thanks, it also seemed to me that way, tried looking into the issues with pyoculus myself but got sidetracked unfortunately.

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