-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add 4th order derivative transforms #586
Conversation
unalmis
commented
Jul 17, 2023
•
edited
Loading
edited
- Adds Zernike radial polynomial fourth order derivative. This is needed for Magnetic axis limits and new variables #556 .
- Ignore division by zero in compute funs.
- Update compute/_core with dummy omega function
The method setter is recursive, not sure what's causing the problem
…On Tue, Jul 18, 2023, 7:15 PM Kaya Unalmis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In desc/transform.py
<#586 (comment)>:
> self._built = False
self._built_pinv = False
- self._set_up()
+ self._derivatives = self._get_derivatives(derivs)
+ self._sort_derivatives()
+ # assign according to logic in setter function
+ self.method = method
+ # assign according to logic in property function
+ self._matrices = self.matrices
Is the def method setter supposed to be recursive? If you notice it calls
itself because it assigns with self.method = rather than self._method.
"Fixing" this to make it not recursive generates similar missing attribute
errors. It's not clear to me why either of these issues occur from making
these changes, but maybe they are related.
I'm hesitant to make your change. Currently it seems the matrices
attribute is getting deleted after *init*, and I think your suggestion
will avoid that by constantly reconstructing it whenever its called.
—
Reply to this email directly, view it on GitHub
<#586 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMFUIRVBM45JTVZLLTZJY3XQ4KI7ANCNFSM6AAAAAA2NPWS74>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| ------------------------------- | ------------------- | ------------------- | ------------------ | ------------------ |
+test_build_transform_fft_lowres | -1.38+/-0.86 | -1.67e-04+/-1.0e-04 | 1.20e-02+/-7.4e-05 | 1.21e-02+/-7.3e-05 |
+test_build_transform_fft_midres | -2.23+/-0.51 | -2.40e-03+/-5.5e-04 | 1.05e-01+/-4.1e-04 | 1.08e-01+/-3.6e-04 |
+test_build_transform_fft_highres | -1.76+/-0.54 | -9.08e-03+/-2.8e-03 | 5.06e-01+/-2.6e-03 | 5.15e-01+/-1.0e-03 |
test_equilibrium_init_lowres | -1.17+/-1.39 | -5.73e-03+/-6.8e-03 | 4.86e-01+/-6.8e-03 | 4.92e-01+/-5.9e-04 |
+test_equilibrium_init_medres | -1.40+/-0.40 | -3.24e-02+/-9.4e-03 | 2.28e+00+/-5.0e-03 | 2.32e+00+/-7.9e-03 |
+test_equilibrium_init_highres | -0.70+/-0.32 | -5.38e-02+/-2.5e-02 | 7.68e+00+/-1.3e-02 | 7.73e+00+/-2.1e-02 |
+test_objective_compile_heliotron | -1.67+/-1.11 | -1.96e-01+/-1.3e-01 | 1.16e+01+/-6.7e-02 | 1.17e+01+/-1.1e-01 |
test_objective_compile_dshape_current | -3.45+/-4.47 | -1.40e-01+/-1.8e-01 | 3.91e+00+/-1.2e-01 | 4.05e+00+/-1.3e-01 |
test_objective_compile_atf | -1.17+/-2.19 | -1.44e-01+/-2.7e-01 | 1.22e+01+/-1.7e-01 | 1.23e+01+/-2.1e-01 |
test_objective_compute_heliotron | -2.45+/-353.25 | -6.01e-04+/-8.7e-02 | 2.39e-02+/-6.0e-02 | 2.45e-02+/-6.2e-02 |
test_objective_compute_dshape_current | -1.45+/-416.78 | -1.59e-04+/-4.6e-02 | 1.08e-02+/-3.2e-02 | 1.10e-02+/-3.3e-02 |
test_objective_compute_atf | -1.31+/-368.22 | -3.07e-04+/-8.6e-02 | 2.31e-02+/-6.0e-02 | 2.34e-02+/-6.1e-02 |
-test_objective_jac_heliotron | +15.97+/-0.41 | +7.83e-01+/-2.0e-02 | 5.68e+00+/-1.3e-02 | 4.90e+00+/-1.6e-02 |
test_objective_jac_dshape_current | -2.42+/-4.55 | -2.93e-03+/-5.5e-03 | 1.18e-01+/-2.0e-03 | 1.21e-01+/-5.1e-03 |
test_objective_jac_atf | -0.44+/-0.90 | -2.33e-02+/-4.8e-02 | 5.31e+00+/-2.3e-02 | 5.33e+00+/-4.2e-02 |
test_perturb_1 | -0.56+/-45.17 | -4.79e-02+/-3.8e+00 | 8.47e+00+/-2.7e+00 | 8.52e+00+/-2.7e+00 |
test_perturb_2 | -0.59+/-3.01 | -8.70e-02+/-4.4e-01 | 1.46e+01+/-3.0e-01 | 1.47e+01+/-3.2e-01 | |
determined to be cause of the bug. The transform matrices attribute is being assigned and is persistent. The error the previous commits were trying to isolate was just an issue with loosing the matrices attribute while saving to output. By calling .matrics property attribute everywhere we do not need a _set_up method.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #586 +/- ##
==========================================
+ Coverage 94.14% 94.21% +0.07%
==========================================
Files 74 74
Lines 17426 17646 +220
==========================================
+ Hits 16405 16626 +221
+ Misses 1021 1020 -1
|
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.
could you add a correctness test for the jacobi derivative 4th order? like some crude check against finite differences or something, just to make sure things are correct.
Made requested change to merge some changes from #556 |
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.
Is there any particular order to the entries in this file? I think at one point they were sorted alphabetically by derivative order and I'd like to keep that if possible to make code generation and maintenance easier.
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.
They have now been sorted using the script I made here: https://github.com/PlasmaControl/DESC/blob/basis_vectors/docs/sort_compute_funs.py
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.
It looks like sorting them alphabetically doubled the diff size. If you want a custom sort order with lower case first that can be done via the one-line modification to the script as discussed here https://stackoverflow.com/a/28136499.
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.
Thanks! The actual order isn't so important as long as there is some logic to it so its reproducible and we can keep it in order.