Added airy disc visualization to spot_diagram.py#51
Added airy disc visualization to spot_diagram.py#51HarrisonKramer merged 8 commits intoHarrisonKramer:masterfrom
Conversation
There was a problem hiding this comment.
Thanks a lot for adding this! This is a great addition.
First, there is already an issue for this feature (as you pointed out), so please assign yourself to that to avoid someone else implementing this too.
A few critical remarks:
- The Airy disk should be centered on the chief ray intersection point on the image plane. You will need to trace a chief ray first, once per field, then center the Airy disk at that location. Also note that the spot centroid is subtracted from the spots before plotting.
- The Airy disk can't be assumed to be a circle (see issue #46). It may be elliptical.
- You use the paraxial F/#, but this is not generally valid. The calculation of the radius (in each axis) will be more complex. The implementation below is one option (feel free to propose an alternative though). If the implementation becomes quite complex, I prefer to move the calculation out of the spot_diagram.py module to keep things simplified and modular.
Possible implementation for Airy radius in each axis (similar to Zemax, I think)
- Trace a chief ray for the given field.
- For the given field, trace rays in the pupil at top/bottom and left/right of pupil, so field coordinates (0, ±1) and (±1, 0).
- Compute angles between chief ray and each marginal ray, define as
$\theta$ . - Compute working F/# as
$N_w = 1 / (2 \cdot n \cdot \sin(\theta))$ , where$n$ is the refractive index in image space (1 by default). - Find the average F/# for the X and Y axes (not sure this is best approach).
- Find the radius in each axis as
$1.22 \cdot N_w \cdot \lambda$ - Plot corresponding ellipse.
Again, not positive this is the best approach, or if it generally valid for all optical systems. Feel free to make a suggestion.
Let me know if anything isn't clear. I can elaborate or provide guidance where needed.
Thanks again!
Kramer
optiland/analysis/spot_diagram.py
Outdated
| num_rings, distribution) | ||
|
|
||
| def view(self, figsize=(12, 4)): | ||
| def view(self, figsize=(12, 4), is_airy_disc = False): |
There was a problem hiding this comment.
- To make the argument more clear, I would propose to use
add_airy_diskinstead ofis_airy_disc. - Please follow PEP 8 guidelines and do not put spaces around the "=" i.e., please write
add_airy_disk=False. It's a small detail, but I prefer the whole codebase follows best practices.
optiland/analysis/spot_diagram.py
Outdated
| Args: | ||
| figsize (tuple): the figure size of the output window. | ||
| Default is (12, 4). | ||
| is_airy_disc (bool): Airy disc visualization controller. |
There was a problem hiding this comment.
update argument name (see above)
optiland/analysis/spot_diagram.py
Outdated
| for k, field_data in enumerate(data): | ||
| self._plot_field(axs[k], field_data, self.fields[k], | ||
| axis_lim, self.wavelengths) | ||
| axis_lim, self.wavelengths, is_airy_disc=is_airy_disc) |
optiland/analysis/spot_diagram.py
Outdated
| plt.tight_layout() | ||
| plt.show() | ||
|
|
||
| def Airy_Radius(self): |
There was a problem hiding this comment.
- Follow PEP 8 and use lowercase words separated by underscores:
airy_radius - More critically, this implementation is not generally valid because it uses the paraxial F/#. We must instead compute the F/# using real rays, so this function needs to change. Depending on how complex that function becomes, it might make sense to move that outside of this class entirely. I prefer to keep functions and classes as simple and modular as possible.
optiland/analysis/spot_diagram.py
Outdated
|
|
||
| def _plot_field(self, ax, field_data, field, axis_lim, | ||
| wavelengths, buffer=1.05): | ||
| wavelengths, buffer=1.05, is_airy_disc=False): |
optiland/analysis/spot_diagram.py
Outdated
| Returns: | ||
| None | ||
| """ | ||
| airy_radius = self.Airy_Radius() |
There was a problem hiding this comment.
move this under the if statement so it's not unnecessarily computed
optiland/analysis/spot_diagram.py
Outdated
|
|
||
| if is_airy_disc: | ||
| # Use a Circle patch instead of manual line plotting | ||
| airy_circle = plt.Circle((0, 0), airy_radius, color='black', fill=False, linewidth=1) |
There was a problem hiding this comment.
small note, but make this a dashed line instead to make it stand out a bit more
|
Thanks for the elaborate review. I need to get more used to the optiland to work with traces. Will read some more documentation and code, and delve into the literature. After figuring out the F# calculation, I can fix the other issues (naming, PEP 8 convention, etc.) and resend the pull request. |
|
It's already a great start. I recognize that this feature has become more complex and is harder to tackle when less familiar with the code base. I'm happy to help where I can. For raytracing, see Tutorial 2a. You can also trace a single ray like this: Using this functionality, you can trace chief and marginal rays, then find the angles between them (i.e., angles between two vectors, where the direction cosine defines the 3-vector). That leads you to the F/# using the approach above, I think. Please check the logic and propose another approach if you find something more suitable. I'm probably missing some edge cases where this might fail. Thanks again! and let me know if I can clarify anything else. Kramer |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
- Coverage 96.92% 96.84% -0.08%
==========================================
Files 105 105
Lines 6209 6316 +107
==========================================
+ Hits 6018 6117 +99
- Misses 191 199 +8
|
… remains the same.
|
All right, there was probably an incompetency between my files, now I get the x and y radii values as ([array([0.41934402]), array([0.41934402]), array([0.41934402])], |
|
Thanks for continuing with this! Impressive work. Will be a great addition. Can you go ahead and push the changes so I can review the code more easily? No problem if you prefer to make a few changes before pushing though. No rush. I also see you (mistakenly?) added a Regards, |
|
I have no idea how poetry.lock is included, tried but can't delete it from the pull request, may I open another project and create a pull request from there? |
|
You should be able to (temporarily) delete your poetry.lock file, commit and push the deletion, then restore your file (if you wish). Maybe you need to close your editor first. I believe I can also do it, although it will be easier for you. Thanks for pushing the changes. I will review now. |
There was a problem hiding this comment.
This looks great!
I did not test your code, but could you confirm that it generates Airy disk radii as expected for a few of the sample lenses? It would be very nice if you added some tests for this, but it won't be blocking the PR.
If you can make the simplification for the angle calculation, then should be good to merge.
Thanks,
Kramer
optiland/analysis/spot_diagram.py
Outdated
| return coordinate_list | ||
|
|
||
|
|
||
| def angle_from_cosine(self, cos1, cos2): |
There was a problem hiding this comment.
I think you can simplify this calculation.
The marginal ray(s) will have a direction unit vector defined by its direction cosines: a = np.array([ray_m.L, ray_m.M, ray_n.N])
Similarly, the chief ray will have a direction unit vector: b = np.array([ray_c.L, ray_c.M, ray_c.N])
Then you can find the angle via np.arccos(np.abs(np.dot(a, b))). The abs limits the angle to 0-90 degrees.
Can you implement the calculation in this way? I think it will simplify your code too.
There was a problem hiding this comment.
This was a pitfall I had been thinking about. As you pointed out, my calculations may not be valid for image planes bigger than \theta 90 degrees. I will implement it as stated, thanks for clarifying.
optiland/analysis/spot_diagram.py
Outdated
| def airy_disc_x_y(self, wavelength): | ||
|
|
||
| """Generates marginal rays, chief rays, then compares the angle between them. | ||
| Averaging each x and y axes, produces F# (N_w), then calculates the airy airy radius at each x-y axes. |
optiland/analysis/spot_diagram.py
Outdated
| return ray_north, ray_south, ray_east, ray_west | ||
|
|
||
| # generate multiple chief ray's angle | ||
| def generate_chief_rays_cosines(self, wavelength): |
There was a problem hiding this comment.
I think you already trace the chief rays once in get_image_coordinates. Would it be possible to only have to trace them once, instead of twice?
There was a problem hiding this comment.
It was a mere attempt to spot the centroid of the rays as x-y coordinates. I was planning to eliminate that function, as it is already calculated by the default centroid(self) function inside spot_diagram.py. I will try to omit that function.
optiland/analysis/spot_diagram.py
Outdated
| field data. | ||
| buffer (float, optional): Buffer factor to extend the axis limits. | ||
| Default is 1.05. | ||
| add_airy_disk (bool, optional): Whether to plot the airy disc or not, for analysis purposes. |
There was a problem hiding this comment.
add comment that Airy disk is centered on the chief ray intersection point on the image plane.
Thanks for the reviews, I can include some snapshots but it may take one more day to get used to the testing environment and add tests to the PR. I may add them after I commit the other requested changes if it is okay. Otherwise, I can implement the tests and then create PR, but as I said it may take a day. |
|
No worries. I think if you're confident in the implementation and show a few examples of it working as intended, then it should be okay to proceed. We can add tests later too. |
|
Update: Tested my calculations with your reviews by a Zemax design (directly imported it into python), and they both give around (x = 5.7 mm, y= 5.7 mm). However, the problem is the normalizing of the plot (Hx, Hy type), trying to figure it out. I will figure it out probably but a help would make it faster. Figures illustrate python output (x, y for two fields) and zemax snapshots for the first field showing . Kind regards, |
|
Very nice. It looks like you're getting close. Can you elaborate on the issue with normalization of the plot? I want to make sure I understand what you mean. I can then help more easily. Does this have something to do with the fact that one of the four values you print is not 5.7369 mm? I assume they should all be equal for an on-axis point. |
|
Firstly, my bad. It correctly gets both x, and y as the python output figure I provided displays x_radii list, then y_radii list (1st row is x axis, 2nd row is y axis. 1st column is for the first field, 2nd column is for the second field). Hence, the 5.66 value is for the second field, which I didn't provide an image for the zemax case, not to crowd here with more figures. Secondly, the problem is I directly input the unnormalized airy radii found to the plotting function (patches.Ellipse). But the original spot diagram plot is done on Hx & Hy. Here are the snapshots with and without airy disc. |
|
I'm still not sure that I full understand the discrepancy. It sounds like it's related to how the code is structured. If you're unable to solve it, I can also help. I can directly modify the pull request if you want me to assist with some of the code. Just let me know if you want some help. It's very close! Regards, |
|
Thanks for sharing the update! It looks really great. The shape/size of the ellipses look consistent, at least qualitatively, so that's good. I recognize the spot centering is a bit tricky. I subtract the centroid for plotting purposes. This implies the ellipse should be centered on the chief ray position minus the centroid. Perhaps that's what you're doing, but I need to see the code to confirm. Can you push the latest changes to this PR so I can take a closer look? I can try to debug. This is very close - I'm sure we can work it out quickly. |
|
Okay, it worked. I will give my messy code a clearer look, then push the latest changes. |
HarrisonKramer
left a comment
There was a problem hiding this comment.
Looks great!
Only one minor change, then should be good to merge.
I may move on with the tests. The real centroids, and the airy radii of x-y may be tested. Also, helper functions may be tested. |
|
All tests are passing, so looks solid. I recognize more tests could be added, but this can be done later on. Test coverage remains high. I really appreciate the contribution! I will merge now. Feel free to make more contributions in the future if interested :) Thanks, |
|
I've just added you as a contributor in the docs as well: |
|
Thanks, I will definitely keep contributing. This one was such a rollercoaster. Thanks for all the help. |









In the analysis.spot_diagram, added an airy disc to let the user analyze their system further.
Key points,