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

Add API and attr support for include/exclude stars for guide #174

Merged
merged 6 commits into from
Nov 29, 2018

Conversation

taldcroft
Copy link
Member

This makes it possible to specify a list of stars to specifically include or exclude for both acq and guide catalogs. This changes the get_aca_catalog API to have separate kwargs for acq and guide.

  • The include/exclude functionality is implemented and working for acq catalog selection.
  • For guide it is only a stub but there are minimal tests showing that it does set the appropriate GuideCatalog include_ids and exclude_ids attributes.

proseco/acq.py Outdated
@@ -154,6 +154,30 @@ def dither(self):
def dither(self, value):
self.dither_acq = value

@property
Copy link
Member Author

@taldcroft taldcroft Nov 28, 2018

Choose a reason for hiding this comment

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

I really should define a descriptor to handle these repetitive definitions.

@taldcroft taldcroft requested a review from jeanconn November 28, 2018 21:27
Argument Description
=================== =========================================================
include_ids_acq list of AGASC IDs of stars to include in acq catalog
include_halfws_acq list of acq halfwidths corresponding to ``include_ids``
Copy link
Contributor

Choose a reason for hiding this comment

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

The '_acq' seems a little odd for the halfw (because we don't handle halfw for guide or fid), but I suppose it is consistent.

@jeanconn
Copy link
Contributor

What is the current acq behavior if you try to include a star that is not allowed? Warn and proceed if it is on the CCD. Warn and ignore if it isn't?

@taldcroft
Copy link
Member Author

It needs to be a star in the ACA FOV (as defined by get_stars()). If not proseco raises an exception.

for include_id in self.include_ids:

@jeanconn
Copy link
Contributor

It looks from the next line like it needs to be a valid candidate and not just on FOV (meaning the user couldn't say, override COLOR1!=0.7 or pick ASPQ=41). I'm OK with that, just keeping in mind use cases for an override from include_ids.

@taldcroft
Copy link
Member Author

This function is making sure the included star is in the candidate acq star table. The next line checks if it is already a candidate acq star, in which case no action is required. If not already a candidate, then it ensures that the star is in stars, which is an unfiltered list of stars in the ACA FOV.

@jeanconn
Copy link
Contributor

Ah. Thanks!

@taldcroft
Copy link
Member Author

And just to really convince myself. 😄

In [19]: aca = get_aca_catalog(8008, include_ids_acq=[31072944], include_halfws_acq=[120], raise_exc=True)

In [20]: aca
Out[20]: 
<ACATable length=12>
 slot  idx     id    type  sz   p_acq    mag    maxmag   yang     zang    dim   res  halfw
int64 int64  int64   str3 str3 float64 float64 float64 float64  float64  int64 int64 int64
----- ----- -------- ---- ---- ------- ------- ------- -------- -------- ----- ----- -----
    0     1        1  FID  8x8   0.000    7.00    8.00   919.83  -844.17     1     1    25
    1     2        5  FID  8x8   0.000    7.00    8.00 -1828.23  1053.81     1     1    25
    2     3        6  FID  8x8   0.000    7.00    8.00   385.81  1697.85     1     1    25
    3     4 31075128  BOT  6x6   0.974    9.35   10.85  -318.30  1202.29    20     1   160
    4     5 31075368  BOT  6x6   0.985    9.13   10.63    53.94   754.67    20     1   160
    5     6 31983336  BOT  6x6   0.985    8.64   10.14   890.59 -1600.44    20     1   160
    6     7 32374896  BOT  6x6   0.983    9.17   10.67  2022.93 -2021.75    20     1   160
    7     8 31463496  GUI  6x6   0.000    9.46   10.96  2026.70  1399.47     1     1    25
    7     9 31076560  ACQ  6x6   0.940    9.70   11.20  -932.85  -354.64    20     1   160
    0    10 32375384  ACQ  6x6   0.927    9.79   11.29  1612.15  -428.32    20     1   160
    1    11 31982136  ACQ  6x6   0.970   10.19   11.69   561.96  -186.48    20     1    60
    2    12 31072944  ACQ  6x6   0.000   12.40   13.90 -2572.86  1225.46    20     1   120

In [21]: aca.acqs.stars.get_id(31072944)['ASPQ1']
Out[21]: 293

@taldcroft
Copy link
Member Author

Any objection to merging? This passes tests on Mac, Ska3-flight (64) and Matlab32.

@taldcroft
Copy link
Member Author

I wish I could test on Windows, but so it goes. I'm planning to tag at 4.1 after merging this, noting that there have been non-trivial changes since 4.0.

@taldcroft taldcroft merged commit 1c14d0e into master Nov 29, 2018
@taldcroft taldcroft deleted the include-exclude branch November 29, 2018 02:12
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