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

tickets/DM-43313: Add danish to ts_imsim #36

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

jbkalmbach
Copy link
Member

Add wep_estimator option to img_closed_loop to enable switching between tie and danish in WEP.

@jbkalmbach jbkalmbach force-pushed the tickets/DM-43313 branch 2 times, most recently from 80eeb36 to ae9b9d0 Compare March 14, 2024 17:07
Copy link
Contributor

@suberlak suberlak left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the update! Do you have a plot showing that both with "tie" and "danish" the default loop converges just fine?

@@ -1037,8 +1062,12 @@ def run_img(
Raw seeing in arcsec.
imsim_log_file : str
Location to save imsim log output.
wep_estimator_method : str
Specify the method used to calculate Zernikes in ts_wep.
Options are "Tie" or "danish".
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that "Tie" starts from uppercase, and "danish" does not? Would it hurt to use "tie" and "danish"?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, reading below I think it's just a typo, as add_argument has tie and danish as I suspected

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, typo. Thanks for the catch. Fixed now.

Add wep_estimator option to img_closed_loop to enable switching between tie and danish in WEP.
@jbkalmbach
Copy link
Member Author

For convergence plots see the document here: https://confluence.lsstcorp.org/display/LTS/WEP+Estimation+Algorithms

@jbkalmbach
Copy link
Member Author

Danish successfully converges for LSST FAM, ComCam and CWFS modes.

Here is CWFS as an example (more plots in document in last comment):
image

@jbkalmbach jbkalmbach merged commit 4b3ba85 into develop Mar 18, 2024
4 checks passed
@jbkalmbach jbkalmbach deleted the tickets/DM-43313 branch March 18, 2024 22:46
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.

3 participants