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

Tweaks to sparkles / yoshi based on cycle 24 work #173

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jun 1, 2022

Description

This PR has a few small updates which facilitate the feasibility studies of cycle 24 proposed targets.

  • Add target name to the report output
  • Fix roll dev logic so max_roll_dev is really the max roll deviation when supplied.
    • It turns out there was a mistake in the API and one method (used only internally) had a roll_dev parameter that was basically redundant.
    • I made this deprecated and added a FutureWarning if it is supplied.
  • Fixed some doc strings mistakes
  • Allowed supplying additional proseco keyword args to run_one_yoshi and convert_yoshi_to_proseco_params. For the cycle 24 studies this allows passing target_name directly.

Interface impacts

roll_dev arg to AcaReviewTable.get_roll_intervals() method is now deprecated and will raise a FutureWarning if supplied.

Testing

Unit tests

  • Mac

Independent check of unit tests by Jean

  • Linux:

Functional tests

Works with https://gist.github.com/taldcroft/92d6d92ed80316a9a66dffbf9379eb52.

@taldcroft taldcroft requested a review from jeanconn June 1, 2022 16:31
sparkles/roll_optimize.py Outdated Show resolved Hide resolved
@jeanconn
Copy link
Contributor

jeanconn commented Jun 1, 2022

Should it also just get the target name if available in get_yoshi_params_from_ocat?

@taldcroft
Copy link
Member Author

Should it also just get the target name if available in get_yoshi_params_from_ocat?

Yes, done.

@taldcroft
Copy link
Member Author

@jeanconn - I fixed the issues you noted.

@jeanconn
Copy link
Contributor

jeanconn commented Jun 1, 2022

Thanks. I also ran this on the somewhat stressing case of obsid 17892 which has no target name. Did not cause any errors or issues with output. I note that in general that convert_yoshi_to_proseco_params doesn't behave well for old observations (< 2012) because chandra_aca.drift complains that the dynamic stuff doesn't work for historical observations that old. But that's not an issue for this code and not an issue for the way we are likely to use sparkles.

@taldcroft taldcroft merged commit f48f000 into master Jun 1, 2022
@taldcroft taldcroft deleted the yoshi-a-couple-more-things branch June 1, 2022 19:38
@javierggt javierggt mentioned this pull request Aug 10, 2022
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