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

VLTI aperture #145

Merged
merged 76 commits into from Nov 15, 2022
Merged

VLTI aperture #145

merged 76 commits into from Nov 15, 2022

Conversation

syhaffert
Copy link
Collaborator

An implementation of the VLTI aperture. Based on the ESO P105 VLTI manual.

@syhaffert syhaffert mentioned this pull request Aug 17, 2022
6 tasks
@ehpor ehpor added the enhancement New feature or request label Aug 17, 2022
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #145 (62584ad) into master (8c29370) will decrease coverage by 0.22%.
The diff coverage is 69.28%.

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   80.93%   80.70%   -0.23%     
==========================================
  Files          95       95              
  Lines        6923     7044     +121     
==========================================
+ Hits         5603     5685      +82     
- Misses       1320     1359      +39     
Impacted Files Coverage Δ
hcipy/aperture/__init__.py 100.00% <ø> (ø)
hcipy/dev.py 100.00% <ø> (ø)
hcipy/optics/glass.py 84.78% <ø> (ø)
hcipy/aperture/generic.py 89.43% <14.28%> (-1.48%) ⬇️
hcipy/aperture/realistic.py 91.11% <64.76%> (-5.46%) ⬇️
hcipy/wavefront_sensing/pyramid.py 73.33% <100.00%> (+8.27%) ⬆️
hcipy/optics/optical_element.py 71.87% <0.00%> (ø)
hcipy/field/grid.py 81.86% <0.00%> (+0.54%) ⬆️

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

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.

  • Baselines vary as function of pointing on the sky. Currently, this function only returns the aperture for zenith pointing.
  • Put pngs of the reference/baseline apertures as a comment.

hcipy/aperture/realistic.py Show resolved Hide resolved
hcipy/aperture/realistic.py Outdated Show resolved Hide resolved
@ehpor ehpor added this to the v0.6.0 release milestone Aug 18, 2022
@syhaffert
Copy link
Collaborator Author

An image of the telescope positions
image

And the baselines
image

@ehpor
Copy link
Owner

ehpor commented Aug 18, 2022

  • Put pngs of the reference/baseline apertures as a comment.

I meant tests/baseline_for_apertures/vlti/pupil_without_spiders.fits.gz and tests/baseline_for_apertures/vlti/pupil.fits.gz and the like. :) But these are great images for reference too!

@syhaffert
Copy link
Collaborator Author

I think the aperture will also change with azimuth. Think about two telescopes, what happens when you observe at the horizon and parallel to the telescopes, you will only see a single aperture.

Should I also add the dOPD to the apertures ? Because the dOPD is Baseline * direction vector.

@ehpor
Copy link
Owner

ehpor commented Oct 25, 2022

I think the aperture will also change with azimuth. Think about two telescopes, what happens when you observe at the horizon and parallel to the telescopes, you will only see a single aperture.

Which is why I said "pointing on the sky" rather than "altitude". 😛

Should I also add the dOPD to the apertures ? Because the dOPD is Baseline * direction vector.

Maybe have a separate function for that rather than integrate it into make_vlti_aperture()? Or at least have a separate function for calculating the dOPDs?

FYI, I would be fine with just having a few sentences in the docstring without any extra implementation for non-Zenith pointing, ie. leave it as it is now. Whichever you feel like doing. The failed tests need to be fixed though.

@syhaffert
Copy link
Collaborator Author

Ok. I can implement both next week. I have already done the coding for the different on-sky pointings. I was thinking you were only talking about altitude because you mentioned zenith. My brain just completely ignored azimuth at that point.

@syhaffert
Copy link
Collaborator Author

All changes have been implemented, tests have been added. All checks have been passed. I think this is ready to be merged.

@ehpor
Copy link
Owner

ehpor commented Nov 7, 2022

You still didn't post the baseline apertures. It makes reviewing much easier. I'll try to get the review in tomorrow or the day after.

The baseline apertures used for the tests: from left to right, top to bottom, pupil, pupil_non_zenith, pupil_non_zenith_without_spiders, pupil_without_spiders:
image

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.

LGTM.

@ehpor ehpor merged commit 3a6c42f into master Nov 15, 2022
@ehpor ehpor deleted the vlti_aperture branch November 15, 2022 17:39
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.

None yet

4 participants