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

Fix missing iteration over the subharmonic matrix #60

Merged

Conversation

KlenM
Copy link
Contributor

@KlenM KlenM commented Nov 22, 2020

Hi,
I think there is a little mistake with a big problem in generating phase screens with subharmonics.
In lines 57-61 we create a 3 by 3 frequency matrix. The matrix cn (line 73) has the same shape.
But on lines 78-79 with the code xrange(0,2) we only iterate over the four elements (0,0), (0,1), (1,0), (1,1), not over the full size of matrix 3 by 3.
This leads to an incorrect calculation of the phase screens. The structure function is very different from what is expected. If we iterate over the entire matrix with code xrange(0, 3), the structure function is much closer to the theoretical one.

@codecov
Copy link

codecov bot commented Nov 22, 2020

Codecov Report

Merging #60 (5b67055) into master (1773d8d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   93.70%   93.70%           
=======================================
  Files          24       24           
  Lines        1191     1191           
=======================================
  Hits         1116     1116           
  Misses         75       75           
Impacted Files Coverage Δ
aotools/turbulence/phasescreen.py 98.38% <100.00%> (ø)

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 1773d8d...5b67055. Read the comment docs.

@matthewtownson
Copy link
Member

Hi Nikita, thanks for adding this. Sorry, its taking a bit longer than usual for us to test and merge changes at the moment, but we're not ignoring you!

@matthewtownson matthewtownson merged commit c93ef2d into AOtools:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants