Add aspheric surface with odd and even terms (#291) #300
Conversation
|
|
There was a problem hiding this comment.
Thanks @mattderstine this is fantastic! I think we can simplify ApshericSurface.jl significantly, but this is looking great overall :)
|
Also need an update to https://github.com/microsoft/OpticSim.jl/blob/main/docs/src/primitives.md for the new surface type(s) |
|
@mattderstine Thank you for spending the time to write this PR. Very nice upgrade to aspherics. We sincerely appreciate the hard work required to understand OpticSim and write this code. Allocation tests are failing, which probably has to do with your use of an abstract type rather than a concrete type, as noted by @friggog. We are fanatical about controlling allocations, even though it is a pain, because any allocations that happen in the innermost ray tracing loop kill multithreaded performance, to the point where single threaded is faster than multithreaded on a 64 core machine. Both @friggog and I have experience coaxing the compiler to avoid allocations. It can be tricky. If you run into problems ping us and we'll be happy to help out. |
|
I have another issue that doesn't make sense to me. I modified julia.jl (new line 27) to include AsphericSurface.jl as Zernike.jl is included. It gives a strange error. I don't understand this code well so I have no clue what to do to fix it. This error only arose after I fixed all the unbound args issues. Any pointers would be appreciated. |
|
Also to flag I think we still need some docs and allocation test updates, don't want to forget these before merging - happy to help as these may be bit more difficult to find |
Co-authored-by: Charlie Hewitt <[email protected]>
Co-authored-by: Charlie Hewitt <[email protected]>
Co-authored-by: Charlie Hewitt <[email protected]>
Co-authored-by: Charlie Hewitt <[email protected]>
Co-authored-by: Charlie Hewitt <[email protected]>
|
Sorry got my syntax wrong, L185-187 should be: partial_prod_step(z::AsphericSurface{T,3,Q,EVEN}, r::T, r2::T) where {T<:Real,Q} = r, r2, 2:2:2Q
partial_prod_step(z::AsphericSurface{T,3,Q,ODD}, r::T, r2::T) where {T<:Real,Q} = one(T), r2, 1:2:(2Q-1)
partial_prod_step(z::AsphericSurface{T,3,Q,ODDEVEN}, r::T, r2::T) where {T<:Real,Q} = one(T), r, 1:1:Q |
Codecov Report
@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 54.56% 54.76% +0.19%
==========================================
Files 78 79 +1
Lines 7079 7194 +115
==========================================
+ Hits 3863 3940 +77
- Misses 3216 3254 +38
Continue to review full report at Codecov.
|
…into pr/mattderstine/300
|
@friggog Could you explain the failure in the Documentation test? I know nothing about the documentation system so I can't begin to figure out how to address the issue. |
Sorry missed this before, you added AsphericSurface somewhere in the docs where it wasn't needed - I just made a commit which should fix it. I'll take a look at the docs once they build to check it's fine, but otherwise I think this all looks good! Awesome work and thanks for sticking with it through the reviews, hopefully you can continue to make great contributions like this to OpticSim.jl 🙂 |
There was a problem hiding this comment.
@BrianGun I'll leave this for you to have a final look and complete - all looks ok to me
@mattderstine thanks again for the contribution!
|
@mattderstine looks great! Thank you for your contribution to OpticSim. |
Adds support for odd polynomical aspheric terms.
Changes the calculation of the aspheric terms for point() and partials() to minimize the number of multiplications
Updates zernikesurface to call asphericsurface internally.
Adds separate methods for even, odd and oddeven calculations of point() and partials() for aspheric surfaces. AsphericSurface is modified to pick the appropriate methods.
New functions are created to directly allocate even, odd and oddeven aspheric surfaces.
Tests are added to check the new methods
Minor changes to other functions to directly call aphericsurface when no zernike terms are possible
Fixes #291
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
ran tests on package. added tests to cover for new functions and methods
Test Configuration(s):
Checklist:
I need help at this point to fix the failing allocations tests.