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.

Sign up for GitHub

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

Jump to bottom

add starcat_date argument to get_observations, get_starcats and get_s… #241

Merged
merged 4 commits into from
Jul 6, 2022
Merged

add starcat_date argument to get_observations, get_starcats and get_s… #241

merged 4 commits into from
Jul 6, 2022

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Jun 10, 2022 •
edited
Loading

Description

The starcat_date is effectively the catalog's unique ID, so it would be convenient to be able to fetch a catalog given starcat_date. This PR does that: it adds a starcat_date argument to get_observations, get_starcats and get_starcats_as_table.

If the starcat_date argument is given, the start time defaults to starcat_date and the stop time defaults to starcat_date + 7 days.

I have not checked, but in principle one can have multiple observations with the same catalog, right? It would be nice to check in those cases.

Interface impacts

Add one optional argument to three functions. This should not affect any existing code.

Testing

A unit test was added to check this functionality on a simple case

Unit tests

  • Mac
  • Linux
  • Windows

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

In [1]: from kadi.commands import get_observations, get_starcats

In [2]: get_observations(starcat_date='2022:001:17:00:58.521', scenario='flight')
Out[2]: 
[{'obsid': 23800,
  'simpos': 75624,
  'obs_stop': '2022:002:01:24:53.004',
  'manvr_start': '2022:001:17:01:02.772',
  'targ_att': (0.177875061, 0.452625075, 0.827436517, 0.280784286),
  'npnt_enab': True,
  'obs_start': '2022:001:17:33:53.255',
  'prev_att': (0.116555575, -0.407948573, -0.759717367, 0.492770009),
  'starcat_date': '2022:001:17:00:58.521',
  'starcat_idx': 171677,
  'source': 'DEC3021A'}]

In [3]: get_starcats(starcat_date='2022:001:17:00:58.521', scenario='flight')
Out[3]: 
[<ACATable length=11>
  slot  idx     id    type  sz    mag    maxmag   yang     zang    dim   res  halfw
 int64 int64  int64   str3 str3 float64 float64 float64  float64  int64 int64 int64
 ----- ----- -------- ---- ---- ------- ------- -------- -------- ----- ----- -----
     0     1        2  FID  8x8    7.00    8.00  -773.14 -1741.99     1     1    25
     1     2        4  FID  8x8    7.00    8.00  2140.38   166.73     1     1    25
     2     3        5  FID  8x8    7.00    8.00 -1826.24   160.26     1     1    25
     3     4 29886088  BOT  6x6    7.20    8.70  -277.35  1178.46    28     1   160
     4     5 29887352  BOT  6x6    8.24    9.84 -1329.24 -2103.75    28     1   160
     5     6 30288600  BOT  6x6    9.07   10.58  1758.15  -270.45    28     1   160
     6     7 30290968  BOT  6x6    9.50   11.02   761.76  1753.00    28     1   160
     7     8 30292744  BOT  6x6    8.47   10.03  1349.94  -786.44    28     1   160
     0     9 30289640  ACQ  6x6    9.44   11.08  1777.10   544.73    28     1   160
     1    10 29892168  ACQ  6x6    9.46   11.08 -1534.55 -1989.80    16     1   100
     2    11 29884784  ACQ  6x6    9.67   11.14   647.71  -848.31    24     1   140]

@javierggt javierggt requested a review from taldcroft June 10, 2022 15:59
@javierggt
Copy link
Contributor Author

javierggt commented Jun 10, 2022 •
edited
Loading

Looking at cases where there is more than one OBSID per catalog, I found OBSID 2983 and 2748, which one gets with:

get_observations(starcat_date='2002:063:15:12:55.566', scenario='flight')

and it turns out that OBSID 2983 has two separate catalogs:

get_observations(obsid=2983, scenario='flight')

The first one is not a real catalog. What's the deal with this one?

get_starcats(obsid=2983, starcat_date='2002:063:15:12:55.566', scenario='flight')

@javierggt javierggt mentioned this pull request Jun 23, 2022
Merged
1 task
taldcroft
taldcroft reviewed Jun 29, 2022
View reviewed changes
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.

Some minor comments, otherwise looks good.

kadi/commands/observations.py
@@ -302,6 +303,7 @@ def get_starcats(start=None, stop=None, *, obsid=None, scenario=None,
the archive.
:param as_dict: bool, False Return a list of dictionaries instead of a list
of ACATable objects.
:param starcat_date: CxoTime-like, None
Copy link
Member

Choose a reason for hiding this comment

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

Needs a description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

kadi/commands/observations.py
@@ -217,11 +217,12 @@ def get_starcats_as_table(start=None, stop=None, *, obsid=None, unique=False,
:param scenario: str, None Scenario
:param cmds: CommandTable, None Use this command table instead of querying
the archive.
:param starcat_date: CxoTime-like, None
Copy link
Member

Choose a reason for hiding this comment

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

Needs description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

kadi/commands/tests/test_commands.py
"""
sc = get_starcats(obsid=8008, scenario='flight')[0]
sc_by_date = get_starcats(starcat_date='2007:002:04:31:43.965', scenario='flight')[0]
assert np.all(sc == sc_by_date)
Copy link
Member

Choose a reason for hiding this comment

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

If you want full coverage of your new code you need to check the case of also supplying start and/or stop, along with raising an exception from no match. I'm not adamant on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for the exception

@javierggt javierggt mentioned this pull request Jul 5, 2022
Merged
5 tasks
@javierggt javierggt merged commit 6e917e8 into master Jul 6, 2022
@taldcroft taldcroft deleted the get-obs-starcat-date branch July 6, 2022 19:26
This was referenced Jul 7, 2022
Closed
Merged
@javierggt javierggt mentioned this pull request Aug 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taldcroft taldcroft taldcroft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@javierggt @taldcroft