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

WIP: Add a force_catalog keyword arg for analysis #309

Closed
wants to merge 1 commit into from
Closed

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Sep 1, 2019

Add a force_catalog keyword arg for analysis

@jeanconn
Copy link
Contributor Author

jeanconn commented Sep 1, 2019

This has been hanging around and needs at least a test.

@taldcroft
Copy link
Member

I'd rather not put this in the API for this rarely-used option. Note that in addition to a test you would need to update all the docs that reference available keywords.

I'd suggest instead to add a function to core.py : includes_from_obsid(obsid) that returns a dict so that you can do:

from proseco.core import includes_from_obsid
get_aca_catalog(obsid, **includes_from_obsid(obsid))

@jeanconn
Copy link
Contributor Author

jeanconn commented Sep 1, 2019

I was thinking that get_aca_catalog(obsid, force_catalog=True) might be more useful than get_aca_catalog(obsid) at this point.

@taldcroft
Copy link
Member

What are the use cases for that? I think all the existing unit tests that reference obsid are correct as written, and in the recent case for SEP0919 prelim I did not see a need to look at the force-catalog version of the 3 reports you supplied.

@jeanconn
Copy link
Contributor Author

jeanconn commented Sep 1, 2019

I wasn't suggesting changing the default (which would apply to the "I think all the existing unit tests that reference obsid are correct as written" comment). I was much more interested in updating the acq stats database (or a new table) to have the last 5 years of proseco P2 values and acq probs for individual stars to do something like sot/starcheck#257 (comment) .

Which is, of course, fine with the helper function idea you suggest. My point was just that I am also not using get_aca_catalog(obsid) much at this point, though of course we may want to do so for large recalibration efforts.

@taldcroft
Copy link
Member

Good points, but not a driver for an API change in my view.

@jeanconn
Copy link
Contributor Author

jeanconn commented Nov 8, 2019

Closing this in favor of #311

@jeanconn jeanconn closed this Nov 8, 2019
@taldcroft taldcroft deleted the forced branch October 4, 2023 00:10
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