Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PR: Single-stage optimization add-ons #301
Changes from 15 commits
1d129e9
8d7b531
96f708b
e68266e
47eedc3
e9acd7a
5f3203e
c1439e4
d3a95b9
5bdb53c
c75c591
a980fac
9127080
7e7fafb
b2857fe
416e2ad
beb070d
11e01b7
6355f80
5b595b1
a0d34c7
6b4f8c8
8e7a0db
a7ab23a
602274a
0493ff6
39d81ed
5ebb987
8f49236
e6cede3
9588839
be922d7
3e9b8f1
c182635
bdacce7
dbf0232
8dafa7c
c8acce0
bedeaf0
7dfa31d
11e6e93
e055240
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
See example here
https://github.com/hiddenSymmetries/simsopt/blob/rj/single_stage_PR/examples/3_Advanced/single_stage_optimization_finite_beta.py#L223
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 bymake_optimizable()
. Most optimizable objects don't have anon_dofs
attribute.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
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 don't understand why this change is needed - can you explain?
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 change forced dofs to be a 1D array. However, this implementation would not work for several dofs given by the user to the optimizable object. To fix this, I reverted back to only dofs.append(arg) and added after the outer loop the lines
# Make sure that dofs is a flat array dofs = np.asarray(dofs).flatten()
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 don't understand why this change is needed - can you explain?
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 was implemented in order to allow dofs to be a 1D array put by the user in the optimizable object. To make it work with both 1 dof and several dofs cases, I changed the line
dofs = self.local_full_x
to
dofs = np.atleast_2d(self.local_full_x)
This should not affect previous runs, only the new ones where dofs would be a 1D array.