Skip to content
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 the GMT and TMT pupil #114

Merged
merged 10 commits into from
Nov 15, 2021
Merged

Add the GMT and TMT pupil #114

merged 10 commits into from
Nov 15, 2021

Conversation

syhaffert
Copy link
Collaborator

I needed the GMT and TMT pupil for some work I am doing at the moment. I implemented both of them. There were no clear definitions of the pupils, so I used multiple sources to come to a best version.

The TMT has the correct shape (number of segments and size) except that the spider width and the central obscuration were fitted from published images.

The GMT pupil is quite difficult to make because of the complex spider structure on the central segment. There is no description of the shadow that the spiders create. The pupil is derived from ZEMAX ray tracing as far as I know. That will make it difficult to have a precise analytical description. I based the sizes of the segments themselves on the design document of the GMT. I compared the implementation with the actual pupil that is used with the GMT Organization (GMTO). This led to minor corrections of the segment sizes and offsets. I also derived the spiders structure from the GMTO pupil file. The file was sampled with 1 cm pixel sizes. So, the current hcipy implementation is correct with errors smaller than 1 cm. The final errors on the sizes are smaller than 0.1%, which I think is an acceptable error margin.

@syhaffert syhaffert requested a review from ehpor November 11, 2021 17:48
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #114 (402456e) into master (f398e82) will increase coverage by 0.30%.
The diff coverage is 94.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   79.99%   80.29%   +0.30%     
==========================================
  Files          94       94              
  Lines        6582     6700     +118     
==========================================
+ Hits         5265     5380     +115     
- Misses       1317     1320       +3     
Impacted Files Coverage Δ
hcipy/aperture/__init__.py 100.00% <ø> (ø)
hcipy/aperture/generic.py 92.65% <76.47%> (+0.53%) ⬆️
hcipy/aperture/realistic.py 95.94% <97.19%> (+0.35%) ⬆️

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 f398e82...402456e. Read the comment docs.

Copy link
Owner

@ehpor ehpor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only formatting changes and one function request.

hcipy/aperture/generic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
@syhaffert syhaffert requested a review from ehpor November 12, 2021 15:30
Copy link
Owner

@ehpor ehpor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the two code paths for the elliptical_aperture() function.

hcipy/aperture/generic.py Show resolved Hide resolved
hcipy/aperture/generic.py Outdated Show resolved Hide resolved
tests/test_aperture.py Outdated Show resolved Hide resolved
hcipy/aperture/generic.py Outdated Show resolved Hide resolved
@ehpor
Copy link
Owner

ehpor commented Nov 12, 2021

I also see that the CI failed on the unrelated thin lens tests, and only for Linux with Python 3.9 for some reason. It's not incidental, since I restarted CI multiple times and it's still there. I'm not gonna merge this before that error is fixed, since that would break CI for the master branch. But that must be done in a separate PR, and this PR will need to be rebased onto master after merging that separate PR.

@syhaffert
Copy link
Collaborator Author

Yeah we need to figure out what's wrong. I went through the changelog if python 3.9 yesterday and I couldn't find an obvious perpetrator. So I will probably have to go through it line by line.

@ehpor
Copy link
Owner

ehpor commented Nov 12, 2021

Yeah we need to figure out what's wrong. I went through the changelog if python 3.9 yesterday and I couldn't find an obvious perpetrator. So I will probably have to go through it line by line.

I created a branch to investigate this: thin_lens_test_failure. See the build here: https://dev.azure.com/ehpor/hcipy/_build/results?buildId=107&view=logs&j=0ac4dd6d-f8e0-5200-4ffd-0e88b8f1d21d&t=d479e7c4-7093-5844-d540-1509ea9134cb. It seems like the peak power becomes nan after the first prop, but only the first one of the two tests (the second one is the same as the first one, but with double the focal length). And only for Linux Python 3.9. Strange...

@ehpor
Copy link
Owner

ehpor commented Nov 12, 2021

Yeah, so it's definitely intermittent. The failure disappeared after a few CI test runs with the test branch. And I just reran this branch, which failed before, and it passes now. I remember seeing this test fail before, so it's something that may depend on the specific machine its running on. I'm copy pasting the relevant part of the failure below, as the runs on Azure get deleted after 30 days.

z [ 0.  3.  6.  9. 12. 15. 18. 21. 24. 27. 30. 33. 36. 39. 42. 45. 48. 51.
 54. 57. 60.]
peak [Field(1.94038034e-05), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan), Field(nan)]
z2 [  0.   6.  12.  18.  24.  30.  36.  42.  48.  54.  60.  66.  72.  78.
  84.  90.  96. 102. 108. 114. 120.]
peak2 [Field(1.94038034e-05), Field(3.35065073e-05), Field(4.375403e-05), Field(6.10293918e-05), Field(8.11504359e-05), Field(0.00011799), Field(0.00047752), Field(0.00038099), Field(0.001278), Field(0.00728061), Field(0.02073922), Field(0.00769245), Field(0.00100904), Field(0.00039627), Field(0.00048316), Field(0.00016863), Field(0.00010752), Field(6.44379246e-05), Field(8.44530374e-05), Field(9.41318839e-05), Field(6.92255138e-05)]

We'll come back to this once it starts failing again, but I'm now sure it's totally unrelated to what you're doing here. I'm fine with merging once you've addressed my comments above.

@ehpor ehpor added the enhancement New feature or request label Nov 14, 2021
hcipy/aperture/generic.py Outdated Show resolved Hide resolved
Copy link
Owner

@ehpor ehpor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do an admin merge tomorrow, after running the tests myself as well.

The failing test is #116 related and master is already failing because of it, so I see no reason for this to wait until after that's fixed.

@ehpor ehpor merged commit 7c2b571 into master Nov 15, 2021
@ehpor ehpor deleted the add_gmt_pupil branch November 15, 2021 16:30
@ehpor
Copy link
Owner

ehpor commented Nov 15, 2021

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants