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

Bar mask transmission #190

Merged
merged 5 commits into from
Feb 20, 2018
Merged

Bar mask transmission #190

merged 5 commits into from
Feb 20, 2018

Conversation

JarronL
Copy link
Contributor

@JarronL JarronL commented Feb 6, 2018

Currently, the bar mask has a transmission of 0 at x=0. Presumably, the intent was to set 0 transmission along the bar’s x-axis (at y=0). The current implementation creates a vertical line that is perpendicular to the wedge rather than through the wedge horizontally. This update should fix that issue.

Incorporate Zernike interpolation from the non-uniform 2D grid data
rather than simply grabbing nearest neighbor.
Currently, the bar mask has a transmission of 0 at x=0. Presumably, the
intent was to set 0 transmission along the bar’s x-axis (at y=0). The
current implementation creates a vertical line that is perpendicular to
the wedge rather than through the wedge horizontally. This update
should fix that issue.
@mperrin
Copy link
Owner

mperrin commented Feb 6, 2018

aw, shoot. yes that was supposed to be y=0. Good catch.

@JarronL
Copy link
Contributor Author

JarronL commented Feb 6, 2018

Still haven't gotten the hang of pull requests. It would be nice if I could send a pull request for only a single commit rather than others that have already been sorted out...

@mperrin
Copy link
Owner

mperrin commented Feb 20, 2018

The Travis tests failed for this. Upon investigation, this failure was because the fix does introduce significant enough changes in the coronagraphic PSFs to just slightly exceed the 1% threshold in the unit test for how much the total post-coronagraphic flux is allowed to differ from the expected value from prior calculations. But it's clear this is a real and necessary fix, and presumably the new value is more accurate than the old one.

So I am going to go ahead and merge this, and will update the unit tests accordingly as part of some related work I'm doing for #191.

@mperrin mperrin merged commit 7d87f7b into mperrin:master Feb 20, 2018
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