Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
+ Coverage   93.48%   93.49%   +0.01%     
==========================================
  Files         375      375              
  Lines       23682    23693      +11     
==========================================
+ Hits        22138    22151      +13     
+ Misses       1544     1542       -2     

@HarrisonKramer, tangentially related to this PR: I've struggled with this much longer than one might think, based on the number of lines that are changed in the core framework. This was mostly because of the bugs with the signs described above, as now sign convention seems to be pretty consistent. I was thinking, would there be room for a tutorial on the sign conventions (if we agree on this PR)? I am thinking of putting it somewhere between the existing 1a 1b tutorials, and could deal with real (spherical) surfaces and paraxial surfaces.
Edit: and I'm volunteering to create it

Hi @rjmoerland,

Thanks for this deepdive and the fix! It's clear there are flaws in the master branch implementation. Your fix looks solid, and the new sign handling makes sense. I think we can already merge this, but let me answer your questions here first.

First, you're right that paraxial raytracing logic was implemented with only simple linear systems in mind. I have come across a few cases, in which the assumptions here have failed. It very likely makes the most sense just to extend the paraxial ray tracing capability to 3D. I need to explore this in more detail, but I agree that it will effectively just be an extension of the 3D real ray tracing engine, but while using the paraxial assumptions.

Adding a ThinLensInteractionModel could also work, though my first instinct is that this might be more complex than needed. Happy to be proven wrong here though. I would like to investigate the possibilities here, then come back to it, if that's okay for you. If I create an issue (or later a PR), I will plan to reference you there too, so you can review any proposals.

And yes, a new tutorial would be great! If it's not clear for you, then it's unlikely it will be clear for the general user. Improving clarity here makes sense. Perhaps we can also update the docs if needed. For example, there's already a section on sign conventions in the cheat sheet.

Thanks again, and let me know if this PR is ready and I will merge.

Kramer

(for the record, I'm gonna answer you, but I'm in the field for work right now so a bit squeezed for time. Same for the other PR! 🙏 )

All good, no rush!

Hi @HarrisonKramer ,

Thanks for this deepdive and the fix! It's clear there are flaws in the master branch implementation. Your fix looks solid, and the new sign handling makes sense. I think we can already merge this, but let me answer your questions here first.

Thanks! I've updated the PR to the new API and resolved the few conflicts. For me this is then good to have merged as well.

Adding a ThinLensInteractionModel could also work, though my first instinct is that this might be more complex than needed. Happy to be proven wrong here though. I would like to investigate the possibilities here, then come back to it, if that's okay for you. If I create an issue (or later a PR), I will plan to reference you there too, so you can review any proposals.

Yes, you're right, that was jumping the gun a bit. What I meant to say is that there seem to be 3 ray tracers: a paraxial ray tracer, a real ray tracer that can trace ParaxialRays, and one that can trace RealRays. It's extra overhead to maintain 3 ray tracers when something changes in the design of the framework, and I also found it somewhat confusing when trying to fix the issues in this PR: the paraxial values come from the paraxial ray tracer, but the graph is generated with RealRays through a surface interaction that may or may not be paraxial. On top of that, the paraxial ray tracers seem to be designed for the in-line configuration, whereas the 3D ray tracer seems to be very capable of tracing through optics at arbitrary locations.

What I like about the various InteractionModels is that they define a real and a paraxial interaction, an approach which I think has a high potential to simplify to a single ray tracer. In the current implementation, as far as I can tell, the interact_real_rays is fully vectorial and takes RealRays, but interact_paraxial_rays is mostly scalar, taking ParaxialRays. I personally would drop the paraxial ray tracers, and always use the vector-based approach, with RealRays (that can then be just called Rays).

In order to get the result of interacting with the real surface, you would trace via each surface and call a method named (for example) interact_real_surface. For paraxial results, you could call something like interact_paraxial_surface. Both would take the RealRays argument, but process them differently. Since this PR implements the paraxial interaction (thin lens model) vectorially for RealRays, it was the source of my enthusiasm to just slap one of those to every surface, but I agree there has to be a more elegant way. Maybe, the implementation for the thin lens model can serve as a start. I would love to see what ideas you yourself come up with, so please do tag me when you're ready.

And yes, a new tutorial would be great! If it's not clear for you, then it's unlikely it will be clear for the general user. Improving clarity here makes sense. Perhaps we can also update the docs if needed. For example, there's already a section on sign conventions in the cheat sheet.

Great! The sign convention you mentioned is pretty clear, so I was thinking of focusing on how the signs of real and the paraxial surfaces are tied together, as it is what I found difficult after I used paraxial mirrors. WDUT?

Thanks again, and let me know if this PR is ready and I will merge.

Ready! 🙏

Cheers,
Robert