-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix curvy bugs #840
Merged
fix curvy bugs #840
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
8e59800
fix bug in PlanarCurve rotation matrix
daniel-dudt 6a37dc4
replace if statement with nan_to_num call
daniel-dudt 3887d6d
update tests
daniel-dudt 3cfad71
make shift & rotmat optimizeable parameters of Curve
daniel-dudt e21202f
noting that fix does not work with AD
daniel-dudt 7c8a040
replace unnecessary jnp with np in Curve class
daniel-dudt d25b289
update examples with private axis curve attributes
daniel-dudt 983aeda
rotation_matrix work with AD at axis=[0, 0, 0]
daniel-dudt 03a0002
ensure shift and rotmat are float arrays
daniel-dudt a88c45a
resolve merge conflicts with master
daniel-dudt 93ce772
repairing tests
daniel-dudt dd432e4
repair plot_coils test
daniel-dudt e236ff8
Merge branch 'master' into dd/curve
ddudt 0b08b9d
Merge branch 'master' into dd/curve
dpanici 1a5c1f2
safenormalize util function to make rotation_matrix work with angle i…
daniel-dudt 8edb49d
revert changes to use angle input again
daniel-dudt eb393ff
merge with master
daniel-dudt d7d0e07
increase test tolerance
daniel-dudt 66b92a9
forgot to revert one change in a test
daniel-dudt 7a6cd48
add FixCurve constraints to maybe_add_self_consistency
daniel-dudt 8f1d3fd
change eq reference to thing
daniel-dudt ac0fc14
better checking for class types based on attrs
daniel-dudt 18ac783
revert change to self-consistent constraint order
daniel-dudt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
You could probably add a cond here that checks if axis is all zero (or otherwise small) and just returns the identity matrix in that case? I think that might solve the AD issue
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.
OK I got that to work, but I had to get rid of the optional
angle
argument and make it always use the norm ofaxis
as the angle. The issue is that the derivative ofjax.linalg.norm(x)
is NaN whenx = 0
, in both forward and reverse mode. So we can't callnorm(axis)
outside of thecond
branch where it is guaranteed to be nonzero.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 think this is what you want:
DESC/desc/compute/utils.py
Lines 492 to 493 in c837cc5
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 we can use
safediv
and keep the angle argument, I prefer that as it is much clearer than having the angle be the norm of the axisThere 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 think I have this working now with the angle argument. I also created a new util function
safenormalize
to normalize vectors to unit length