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 get_effective_t_ccd #309

Merged
merged 4 commits into from
Jun 11, 2019
Merged

Use get_effective_t_ccd #309

merged 4 commits into from
Jun 11, 2019

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Jun 7, 2019

Use get_effective_t_ccd

Starcheck needs the effective_t_ccd a bunch of places anyway, so I'm not sure if we win anything by doing the get_aca_catalog transition. I'll work that in another branch.

Of course, if I'm calling get_effective_t_ccd, the trick is to confirm I'm not accidentally calling it twice on any input temperature.

@jeanconn
Copy link
Contributor Author

jeanconn commented Jun 8, 2019

Though all the uglier parts are probably where we need review...

@taldcroft
Copy link
Member

Just wondering if it is possible to use the aca object from get_aca_catalog to populate the Obsid object with guide and acq effective temperatures, and then use those where appropriate. I.e. don't change the low-level helper functions, but have the routines that call them decide which temperature to pass. This should remove any possibility of double-effectiving because it is never directly called in starcheck. Then in the info statement just check if t_ccd != t_ccd_eff and issue a statement if so. That decouples the details of computing effective temperature from the perl code to issue an info statement.

Anyway, having said that, what you have here will work just fine and I'm OK with it. PITEOG.

@taldcroft
Copy link
Member

Oh, and if you go with the current approach then I agree you should just close #310.

@jeanconn
Copy link
Contributor Author

jeanconn commented Jun 8, 2019

Though #310 just moves things in the right direction and could go as well. It might be a good first step toward working with an ACACatalogTable in starcheck anyway.

@jeanconn
Copy link
Contributor Author

jeanconn commented Jun 8, 2019

Also, whatever we do what do you think about testing? I have played both with changing all of the temperatures (just adding 2 degrees in the original assignments) and played with changing the planning limits to something lower to use old schedules.

@jeanconn jeanconn changed the title WIP: Use get_effective_t_ccd Use get_effective_t_ccd Jun 10, 2019
Replace get_acq_catalog with get_aca_catalog and cleanup
@taldcroft
Copy link
Member

That testing is fine.

@jeanconn
Copy link
Contributor Author

I looked more at getting the effective temperatures back from the ACACatalogTable, but given we would like this for this week, I'm thinking of just not fiddling more with changing the API of anything that goes across the Perl / Python divide. Is this approved for now with the PITEOG in mind?

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Good to go! The implementation can be beautified later.

@jeanconn jeanconn merged commit 8c90c23 into master Jun 11, 2019
@jeanconn jeanconn deleted the effective_t_ccd branch June 11, 2019 19:35
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