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

Use hardware bandpass for sky level #470

Merged
merged 11 commits into from
May 14, 2024
Merged

Conversation

jchiang87
Copy link
Collaborator

This addresses #453

@jchiang87 jchiang87 requested a review from jmeyers314 May 13, 2024 00:19
Copy link
Member

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I'm wondering about the best way to set RUBIN_AREA. From the default config here, photons are initially sampled from an annulus with an effective diameter of d = 2 * sqrt(4.18**2 - 2.55*2) = 703 cm, not 642 or 649 cm. I'm wondering if that later figure is some kind of FOV averaged effective diameter that integrates the vignetting. Since the vignetting is emergent, I'd have guessed we want to use the 703 cm figure for determining the initial flux.

@jchiang87
Copy link
Collaborator Author

jchiang87 commented May 13, 2024

Based on the description in SMTN-002, it looks like we should be using 703 cm, or perhaps more properly, computing the diameter from the values in that config.

@jmeyers314
Copy link
Member

Yep, I agree. I wonder what the best way to compute this programmatically is... We could put the inner/outer diameter into eval_variables, and then wire those up consistently to both PupilAnnulusSampler and SkyModel(here) maybe?

Using 703 cm as the default seems reasonable to me. (I suspect the median-over-FOV value is closer to this than to 642).

@jchiang87
Copy link
Collaborator Author

an effective diameter of d = 2 * sqrt(4.18**2 - 2.55*2) = 703 cm, not 642 or 649 cm.

There's a typo here. Using d = 2 * sqrt(4.18**2 - 2.55**2), I get d = 662.4 cm.

@jchiang87
Copy link
Collaborator Author

jchiang87 commented May 13, 2024

I'll also fix the pupil area in the skycat.py and instcat.py code in this PR.

@jchiang87
Copy link
Collaborator Author

@jmeyers314 I've hooked up everything to use the eval_variables for the pupil annulus radii. Can you have another look?

@@ -39,6 +39,9 @@ eval_variables:
type: Degrees
theta: { type: OpsimData, field: rotTelPos }

fpupil_R_outer: 4.18
fpupil_R_inner: 2.55 # M1 inner diameter is 2.558, but we need a bit of slack for off-axis rays

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just put fpupil_area here once rather than duplicating the code?

Copy link
Collaborator Author

@jchiang87 jchiang87 May 14, 2024

Choose a reason for hiding this comment

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

Sure, I had thought that we might want to access those values individually at some point, but the skycat and instcat implementations indeed probably don't need that.

Copy link
Member

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

LGMT. One last comment/question.

@jchiang87 jchiang87 merged commit ce342d8 into main May 14, 2024
3 checks passed
@jchiang87 jchiang87 deleted the u/jchiang/fix_skybg_throughput branch May 14, 2024 19:16
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