Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

(Fake) sequential raytracing support (#63) #101

Merged
merged 9 commits into from
Apr 12, 2021
Merged

Conversation

cdhf
Copy link
Contributor

@cdhf cdhf commented Apr 9, 2021

Fixes #63. This is not real sequential raytracing, because the system still has to find the correct element the ray hits next. But it allows to select specific beam paths in a complex optical system with eg beamsplitters and makes typical analyses in such systems easier.

@ghost
Copy link

ghost commented Apr 9, 2021

CLA assistant check
All CLA requirements met.

@BrianGun
Copy link
Contributor

BrianGun commented Apr 9, 2021

@cdhf thanks for making this change, it will be useful to many people.

Could you sign the Contributor License Agreement? We can't accept your PR otherwise, it's a Microsoft requirement. If you don't want to sign the CLA then we can submit the PR for you.

@BrianGun
Copy link
Contributor

BrianGun commented Apr 9, 2021

@cdhf have you run the unit tests on your local machine? They are failing on github. Looks like one of the errors occurs at line 186 in processintersection, in Fresnel.jl, called from Comparison.jl in the test sets, somewhere around line 122.

The error is "opticalinterface is not defined". You can see the errors by clicking on the Details button in the box above this one that says "Some Checks were not successful". You will have to scroll quite a ways to get to the error.

I believe it's the same error on both ubuntu and windows.

I looked at that line of code in your repo and don't understand how opticalinterface could be undefined there since it appears to be defined just above. But something is definitely not right.

Also noticed that you redefined the SphericalLens, etc., functions to take an interfacemode argument. This will set the interfacemode for both front and back surfaces the same. Is this what you wanted?

Does this make sense in general or should we have two interface modes, one for the front and one for the back element, both defaulting to ReflectOrTransmit? I don't have an opinion on this, it's more a matter of what the average (if there is such a thing) optical engineer would find most reasonable and useful.

@BrianGun
Copy link
Contributor

BrianGun commented Apr 9, 2021

@cdhf not sure how experienced you are in Julia. Ignore this if you already know it. To run the unit tests on your local machine type ] at the repl to enter the package mode. Then type test.

@cdhf
Copy link
Contributor Author

cdhf commented Apr 10, 2021

It was a typo. I think I forgot to run the tests after changing the conditionals. The tests now work on my machine.

With regard to different interfacemode for front and backside of lenses:
This addresses my more more important use case, so for now it is sufficient for me.

Separate front/backside behaviour makes sense eg to analyse ghost reflections in high power ultra short pulse laser applications. But at least in Zemax I usually had to use this only to work around the bad usability of ray splitting. I sincerely hope that OpticSim will solve that problem at some point. But there is more infrastructure needed for this kind of application, regardless of whether you have raysplitting or not.

@codecov-io
Copy link

codecov-io commented Apr 10, 2021

Codecov Report

Merging #101 (0678b66) into main (6253fba) will increase coverage by 0.00%.
The diff coverage is 76.19%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #101   +/-   ##
=======================================
  Coverage   47.33%   47.34%           
=======================================
  Files          57       57           
  Lines        6621     6622    +1     
=======================================
+ Hits         3134     3135    +1     
  Misses       3487     3487           
Impacted Files Coverage Δ
src/Optical/Lenses.jl 37.16% <73.33%> (ø)
src/Optical/OpticalInterface.jl 67.53% <75.00%> (+0.42%) ⬆️
src/Optical/Fresnel.jl 96.10% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6253fba...0678b66. Read the comment docs.

@friggog friggog changed the title Fakeseq (Fake) sequential raytracing support (#63) Apr 12, 2021
@alfredclwong alfredclwong requested a review from BrianGun April 12, 2021 10:14
@BrianGun BrianGun merged commit 3b4150a into microsoft:main Apr 12, 2021
@cdhf cdhf deleted the fakeseq branch April 13, 2021 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequential raytracing
3 participants