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

Remove timelines / cmd_states.db3 from mica #279

Merged
merged 14 commits into from
Apr 20, 2023
Merged

Remove timelines / cmd_states.db3 from mica #279

merged 14 commits into from
Apr 20, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jan 22, 2023

Description

This removes the cmd_states.db3 (and timelines) dependence from mica.

I'm not sure if any of these changes impact or break processing that is not covered in unit tests. This definitely needs close review by @jeanconn.

Interface impacts

  • It's possible that some old (pre-2003) queries may give different results.
  • The status fields have been reduced/simplified and might give different results from the previous version.
  • The catalog column of the Table from get_monitor_windows() is now always None while previously it contained the starcheck catalog dict (e.g. from get_starcheck_catalog_at_date()).

Testing

Unit tests

  • Mac
  • Linux (kady)

Independent check of unit tests by Jean

  • Linux (fido)

Functional tests

None but many new unit tests.

@taldcroft taldcroft requested a review from jeanconn January 22, 2023 21:51
@taldcroft taldcroft changed the title WIP: Remove timelines from starcheck Remove timelines from starcheck Jan 22, 2023
@taldcroft taldcroft changed the title Remove timelines from starcheck Remove timelines / cmd_states.db3 from mica Jan 22, 2023
@taldcroft
Copy link
Member Author

@jeanconn - I think I have addressed the comments. In addition I ran the unit tests on kady and all passed.

@jeanconn
Copy link
Contributor

jeanconn commented Apr 6, 2023

Also somewhat fundamentally, the obsid from kadi observations does not match starcheck obsids for the vehicle-only portions of schedules. I added a helper function to just make it work, but this is slower and uglier still. But since you started this as very much an incremental patch to mica to strip out timelines, maybe it makes sense. So let me know what you think makes sense for c052566

@jeanconn
Copy link
Contributor

jeanconn commented Apr 6, 2023

It might make more sense to use the kadi observations more directly as a time -> products mapping and avoid the double lookups.

@jeanconn jeanconn self-assigned this Apr 10, 2023
@taldcroft
Copy link
Member Author

@jeanconn - I think the new function is good now and well-tested so let's go with that. From my perspective this PR is good to go so you just need to finish the review.

)["obsid"]
result = starcheck_db.fetchone(
"SELECT starcheck_obs.obsid FROM starcheck_obs "
"INNER JOIN starcheck_id "
Copy link
Contributor

@jeanconn jeanconn Apr 13, 2023

Choose a reason for hiding this comment

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

Hah. I thought you never wanted to see (real / functional) sql in our code again @taldcroft or I'd have done this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well if we're using SQL then it might as well be as efficient as possible. In truth for this particular application the SQL database is not a bad solution, especially if you really want every catalog including planned-never-approved-or-run ones.

If we were doing this today we might think about a shelf of Python dicts (with all the info) keyed by the MP_STARCAT date and maybe MP dir.

@jeanconn
Copy link
Contributor

This works on flight.

In [1]: import mica.starcheck

In [2]: mica.starcheck.get_starcheck_catalog_at_date('2023:099:04:21:40.719')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 1
----> 1 mica.starcheck.get_starcheck_catalog_at_date('2023:099:04:21:40.719')

File /proj/sot/ska/jeanproj/git/mica/mica/starcheck/starcheck.py:203, in get_starcheck_catalog_at_date(date, starcheck_db)
    197 mp_dir = load_name_to_mp_dir(obs["source"])
    199 # Kadi observations are labeled by the commanded obsid.  Starcheck catalogs
    200 # are labeled by the planned obsid used in the starcheck output.  For observations
    201 # after SCS107 with SOSA, the kadi observation obsids and starcheck obsids don't match.
    202 # Use a helper function to line these up by starcat time.
--> 203 obsid = get_starcheck_db_obsid(obs["starcat_date"], mp_dir)
    205 # Use the obsid and the known products directory to use the more generic
    206 # get_starcheck_catalog to fetch the right one from the database
    207 cat_info = get_starcheck_catalog(obsid, mp_dir=mp_dir, starcheck_db=starcheck_db)

File /proj/sot/ska/jeanproj/git/mica/mica/starcheck/starcheck.py:315, in get_starcheck_db_obsid(mp_starcat_time, mp_dir, starcheck_db)
    307 result = starcheck_db.fetchone(
    308     "SELECT starcheck_obs.obsid FROM starcheck_obs "
    309     "INNER JOIN starcheck_id "
   (...)
    312     f"AND starcheck_obs.mp_starcat_time = '{mp_starcat_time}'"
    313 )
    314 if result is None:
--> 315     raise ValueError(f"no starcheck entry for {mp_starcat_time=} and {mp_dir=}")
    317 obsid = result["obsid"]
    318 return obsid

ValueError: no starcheck entry for mp_starcat_time=masked and mp_dir='/2023/APR0323/oflsa/'

@jeanconn
Copy link
Contributor

I suppose technically I should have said that "doesn't throw an unhandled exception" on flight. I'm not sure if flight really has the right answer. It looks like maybe kadi observations use a masked starcat date for the gyro holds? That make sense. For this application maybe it makes sense to just use that obsid instead of the date to return an empty-ish starcheck entry?

@jeanconn jeanconn removed their assignment Apr 19, 2023
@taldcroft taldcroft merged commit d19dc0e into master Apr 20, 2023
@taldcroft taldcroft deleted the no-timelines branch April 20, 2023 17:43
@taldcroft
Copy link
Member Author

I noticed today this additional interface change, which I have put into the top description:

The catalog column of the Table from get_monitor_windows() is now always filled with None while previously it contained the starcheck catalog dict (e.g. from get_starcheck_catalog_at_date()).

This was referenced Jun 1, 2023
@javierggt javierggt mentioned this pull request Jul 12, 2023
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