Add Keck aperture

[vkooten]

This adds Keck aperture ~and lyot stop~ for Keck 2. It solves part of #134 and #153

Picture of aperture:

[codecov[bot]]

Codecov Report

Merging #155 (303b20b) into master (8a84f3e) will decrease coverage by 0.07%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   80.93%   80.86%   -0.07%     
==========================================
  Files          95       95              
  Lines        6923     7103     +180     
==========================================
+ Hits         5603     5744     +141     
- Misses       1320     1359      +39     

Impacted Files	Coverage Δ
hcipy/aperture/__init__.py	100.00% <ø> (ø)
hcipy/aperture/realistic.py	91.84% <ø> (-4.73%)	:arrow_down:

... and 7 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ivalaginja]

• [ ] Put a PNG of the baseline aperture (as checked by the Pytest for an easy check for us and documentation).

@ehpor do you really mean a PNG with this or the fits.gz files in the baseline folder of the tests? I wasn't able to find other PNG references.

Sorry for the billion commits and tests, I am flying a little blind since this branch is on a fork (that's not mine) and I didn't want to install the fork in dev mode, in a new env and so on.

I should be almost done with things, the tests pass when I copy-paste and run them locally. As for the tutorial notebook, I added what I think needs to be there but because of not importing the forked package I wasn't able to run it.

Happy to fix things if there is still something open or something went wrong.

[ivalaginja]

Two of the new tests are still failing, let me look into it...

[ivalaginja]

@ehpor please advise: the two tests that are failing do so because the reconstruction of the aperture from the segments does not have the central obscuration, while the original aperture does when with_spiders is True. This seems to be because Keck, as opposed to LUVOIR A, TMT and ELT does not generate the central obscuration by getting rid of a subset of segments in the center, instead it just uses a circular aperture.

This also made me notice how the Keck aperture uses the with_spiders keyword to control the presence of the spiders and central obscuration at the same time, which is not what the other telescopes do. There, the central obscuration is always there. Even when I change this so that the central obscuration is not tethered to that boolean and the obscuration is always there, the tests fail (now four of them instead of only two) because the from-segment reconstruction is not adding the central obscuration.

The simplest solution I see is to (a) always include the central obscuration in Keck, independent of with_spiders and (b) add the central obscuration to the reconstruction from segments in the test.

Thoughts?

Edit: I just realized the solution I suggested above is not really possible because it would require the function check_aperture() to be able to identify the case in which it's testing case, and we'd need to put the diameter of the central obscuration in there. I'll stand by until I hear from you Emiel.

[ehpor]

@ivalaginja With the PNG, I literally mean a screenshot of DS9 of the fits file that serves as the baseline. In the past, even a glance at that file showed that the aperture was completely wrong. And people just let the code generate an aperture and assume that because the test passes, that they've implemented the Keck or VLT aperture correctly.

[ivalaginja]

@ivalaginja With the PNG, I literally mean a screenshot of DS9 of the fits file that serves as the baseline. In the past, even a glance at that file showed that the aperture was completely wrong. And people just let the code generate an aperture and assume that because the test passes, that they've implemented the Keck or VLT aperture correctly.

Where are those though? I wasn't able to locate them on the repo.

[ehpor]

In tests/baseline_for_apertures/keck/*. They're added by this PR.

[ehpor]

@ehpor please advise: the two tests that are failing do so because the reconstruction of the aperture from the segments does not have the central obscuration, while the original aperture does when with_spiders is True. This seems to be because Keck, as opposed to LUVOIR A, TMT and ELT does not generate the central obscuration by getting rid of a subset of segments in the center, instead it just uses a circular aperture.

This also made me notice how the Keck aperture uses the with_spiders keyword to control the presence of the spiders and central obscuration at the same time, which is not what the other telescopes do. There, the central obscuration is always there. Even when I change this so that the central obscuration is not tethered to that boolean and the obscuration is always there, the tests fail (now four of them instead of only two) because the from-segment reconstruction is not adding the central obscuration.

The simplest solution I see is to (a) always include the central obscuration in Keck, independent of with_spiders and (b) add the central obscuration to the reconstruction from segments in the test.

Thoughts?

Edit: I just realized the solution I suggested above is not really possible because it would require the function check_aperture() to be able to identify the case in which it's testing case, and we'd need to put the diameter of the central obscuration in there. I'll stand by until I hear from you Emiel.

The segments that are returned should be the illuminated part of the segment, not the full segment itself. Therefore, the segments in the first ring of Keck should contain the central obscuration. In principle, the sum of all the segments should be equal to the returned segments. This is what the test is checking for. This would be a few extra lines at the end of the function:

if return_segments:
    def segment_with_central_obscuration(segment):
        return lambda grid: segment(grid) * (1 - make_circular_aperture(central_obscuration_diameter)(grid))
    segments = [segment_with_central_obscuration(segment) for segment in segments]

or something like this without typos.

[ivalaginja]

In tests/baseline_for_apertures/keck/*. They're added by this PR.

@ehpor just to clarify: this is supposed to be a plain png file, uncompressed? Any preferences on the file name? pupil.png, keck.png, keck_pupil.png? Am I seeing right that none of the other folders in baseline_for_apertures contains such a png file yet?

[ivalaginja]

I don't understand. Everything passes locally and I have no uncommitted changes except for the png file and the .DS_Store files, which is completely irrelevant.

[ivalaginja]

Ah. The old reference files do not have the central obscuration in some cases, while what I compare to locally does.

[ivalaginja]

There we go.

[ivalaginja]

Ready for review @ehpor

[ehpor]

Flake8 doesn't report that it finished. This has to do with reviewdog, I think, see #174. I manually checked flake8 and it seems fine. Also, flake8 itself finished successfully on CI. I'm forcing a merge as admin.

[ehpor]

Thanks for the contribution @vkooten! 🎉