-
Notifications
You must be signed in to change notification settings - Fork 55
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
PR: Single-stage optimization add-ons #301
Conversation
Update branch
@@ -253,6 +258,7 @@ def _jac(self, x: RealArray = None): | |||
x = xs[:, j] | |||
mpi.comm_groups.bcast(x, root=0) | |||
opt.x = x | |||
self.opt.non_dofs = non_dofs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the non_dofs attribute used in the optimizable object? Is there any convention or standardization you have in mind here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that it should be an array of doubles. I have no other use cases in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells funny to me, because non_dofs
is an attribute only of objects created by make_optimizable()
. Most optimizable objects don't have a non_dofs
attribute.
Update branch with latest simsopt source code
For a single-stage example with dofs and non-dofs, check |
Ready to be reviewed again |
src/simsopt/_core/optimizable.py
Outdated
@@ -93,6 +93,8 @@ def __init__(self, | |||
""" | |||
if x is None: | |||
x = np.array([]) | |||
elif np.isscalar(x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not added the tests don't pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which test exactly doesn't pass? What exactly is the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now not needed as dofs are now handled correctly in J
I am not really sure why the tests are not passing, @landreman @mbkumar could you take a look? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
=======================================
Coverage 91.34% 91.35%
=======================================
Files 68 68
Lines 12009 12042 +33
=======================================
+ Hits 10970 11001 +31
- Misses 1039 1041 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
options for SquaredFlux definition
All comments addressed, now waiting for the tests to finish. |
Updated branch with latest SIMSOPT master branch |
c0962fe
to
be922d7
Compare
I found a way of keeping the old dofs and non-dofs structure. The single stage example has a few changes but now the whole simsopt has fewer changes and keeps the same old dofs and non-dofs structure. Hopefully all tests pass now. |
Yet another single-stage refactor
The pull request introduces a few features to allow single-stage optimizations to be performed using SIMSOPT. In particular, these are the minimal set of changes needed to fully reproduce the results in https://arxiv.org/abs/2302.10622.
Brief list of changes: