Improve roll options including uniform sampling of roll range

.github/workflows/flake8.yml

@@ -9,11 +9,11 @@ jobs:
 
     steps:
     - uses: actions/checkout@v1
-    - name: Set up Python 3.6
+    - name: Set up Python 3.8
       uses: actions/setup-python@v1
       with:
-        python-version: 3.6
+        python-version: 3.8
     - name: Lint with flake8
       run: |
         pip install flake8
-        flake8 . --count --ignore=W504,E402,F541 --max-line-length=100 --show-source --statistics
+        flake8 . --count --ignore=W503,E402,F541 --max-line-length=100 --show-source --statistics

sparkles/core.py

@@ -81,8 +81,8 @@ def main(sys_args=None):
 
 
 def run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=None,
-                   report_level='none', roll_level='none', loud=False, obsids=None,
-                   open_html=False, context=None, raise_exc=True):
+                   report_level='none', roll_level='none', roll_args=None,
+                   loud=False, obsids=None, open_html=False, context=None, raise_exc=True):
     """Do ACA load review based on proseco pickle file from ORviewer.
 
     The ``load_name`` specifies the pickle file from which the ``ACATable``
@@ -110,13 +110,40 @@ def run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=Non
     default is "none", meaning no reports are generated.  A final option is
     "all" which generates a report for every obsid.
 
+    The ``roll_level`` arg specifies the message category at which the review
+    process will also attempt to find and report on available star catalogs for
+    different roll options. The available categories are the same as for
+    ``report_level``, with the most common choice being "critical".
+
+    The ``roll_args`` arg specifies an optional dict of arguments used in the
+    call to the ``get_roll_options`` method. Possible args include:
+
+    - ``min_improvement``: min value of improvement metric to accept option
+      (default=0.3)
+    - ``d_roll``: delta roll for sampling available roll range (deg, default=0.25
+        for uniq_ids method and 0.5 for uniform method)
+    - ``max_roll_dev``: maximum roll deviation (deg, default=max allowed by pitch)
+    - ``method``: method for determining roll intervals ('uniq_ids' | 'uniform').
+      The 'uniq_ids' method is a faster method that frequently finds an acceptable
+      roll option, while 'uniform' is a brute-force search of the entire roll
+      range at ``d_roll`` increments. If not provided, the default is to try
+      *both* methods in order, stopping when an acceptable option is found.
+    - ``max_roll_options``: maximum number of roll options to return (default=10)
+
+    If roll options are returned then they are sorted by the following keys:
+
+    - Number of warnings at ``roll_level`` or worse (e.g. number of criticals)
+      in ascending order (fewer is better)
+    - Improvement in descending order (larger improvement is better)
+
     :param load_name: name of loads
     :param acars: list of ACAReviewTable objects (optional)
     :param make_html: make HTML output report
     :param open_html: open the HTML output in default web brower
     :param report_dir: output directory
     :param report_level: report level threshold for generating acq and guide report
     :param roll_level: level threshold for suggesting alternate rolls
+    :param roll_args: None or dict of arguments for ``get_roll_options``
     :param loud: print status information during checking
     :param obsids: list of obsids for selecting a subset for review (mostly for debug)
     :param is_ORs: list of is_OR values (for roll options review page)
@@ -128,8 +155,8 @@ def run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=Non
     try:
         _run_aca_review(load_name=load_name, acars=acars, make_html=make_html,
                         report_dir=report_dir, report_level=report_level,
-                        roll_level=roll_level, loud=loud, obsids=obsids,
-                        open_html=open_html, context=context)
+                        roll_level=roll_level, roll_args=roll_args,
+                        loud=loud, obsids=obsids, open_html=open_html, context=context)
     except Exception:
         if raise_exc:
             raise
@@ -141,8 +168,8 @@ def run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=Non
 
 
 def _run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=None,
-                    report_level='none', roll_level='none', loud=False, obsids=None,
-                    open_html=False, context=None):
+                    report_level='none', roll_level='none', roll_args=None,
+                    loud=False, obsids=None, open_html=False, context=None):
 
     if acars is None:
         acars = get_acas_from_pickle(load_name, loud)
@@ -153,6 +180,9 @@ def _run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=No
     if not acars:
         raise ValueError('no catalogs founds (check obsid filtering?)')
 
+    if roll_args is None:
+        roll_args = {}
+
     # Make output directory if needed
     if make_html:
         # Generate outdir from load_name if necessary
@@ -179,9 +209,32 @@ def _run_aca_review(load_name=None, *, acars=None, make_html=True, report_dir=No
         aca.set_stars_and_mask()  # Not stored in pickle, need manual restoration
         aca.check_catalog()
 
+        # Find roll options if requested
         if roll_level == 'all' or aca.messages >= roll_level:
+            # Get roll selection algorithms to try
+            max_roll_options = roll_args.pop('max_roll_options', 10)
+            methods = roll_args.pop(
+                'method', ('uniq_ids', 'uniform') if aca.is_OR else 'uniq_ids')
+            if isinstance(methods, str):
+                methods = [methods]
+
             try:
-                aca.get_roll_options()  # sets roll_options, roll_info attributes
+                # Set roll_options, roll_info attributes
+                for method in methods:
+                    aca.roll_options = None
+                    aca.roll_info = None
+                    aca.get_roll_options(method=method, **roll_args)
+                    aca.roll_info['method'] = method
+
+                    # If there is at least one option with no messages at the
+                    # roll_level (typically "critical") then declare success and
+                    # stop looking for roll options.
+                    if any(not roll_option['acar'].messages >= roll_level
+                           for roll_option in aca.roll_options):
+                        break
+
+                aca.sort_and_limit_roll_options(roll_level, max_roll_options)
+
             except Exception:  # as err:
                 err = traceback.format_exc()
                 aca.add_message('critical', text=f'Running get_roll_options() failed: \n{err}')
@@ -444,7 +497,7 @@ def __init__(self, *args, **kwargs):
             self.rename_column('idx_temp', 'idx')
 
     def run_aca_review(self, *, make_html=False, report_dir='.', report_level='none',
-                       roll_level='none', raise_exc=True):
+                       roll_level='none', roll_args=None, raise_exc=True):
         """Do aca review based for this catalog
 
         The ``report_level`` arg specifies the message category at which the full
@@ -454,10 +507,30 @@ def run_aca_review(self, *, make_html=False, report_dir='.', report_level='none'
         default is "none", meaning no reports are generated.  A final option is
         "all" which generates a report for every obsid.
 
+        The ``roll_level`` arg specifies the message category at which the review
+        process will also attempt to find and report on available star catalogs for
+        different roll options. The available categories are the same as for
+        ``report_level``, with the most common choice being "critical".
+
+        The ``roll_args`` arg specifies an optional dict of arguments used in the
+        call to the ``get_roll_options`` method. Possible args include:
+
+        - ``min_improvement``: min value of improvement metric to accept option
+          (default=0.3)
+        - ``d_roll``: delta roll for sampling available roll range (deg, default=0.25
+            for uniq_ids method and 0.5 for uniform method)
+        - ``max_roll_dev``: maximum roll deviation (deg, default=max allowed by pitch)
+        - ``method``: method for determining roll intervals ('uniq_ids' | 'uniform').
+          The 'uniq_ids' method is a faster method that frequently finds an acceptable
+          roll option, while 'uniform' is a brute-force search of the entire roll
+          range at ``d_roll`` increments. If not provided, the default is to try
+          *both* methods in order, stopping when an acceptable option is found.
+
         :param make_html: make HTML report (default=False)
         :param report_dir: output directory for report (default='.')
         :param report_level: report level threshold for generating acq and guide report
         :param roll_level: level threshold for suggesting alternate rolls
+        :param roll_args: None or dict of arguments for ``get_roll_options``
         :param raise_exc: if False then catch exception and return traceback (default=True)
         :returns: exception message: str or None
 
@@ -467,7 +540,7 @@ def run_aca_review(self, *, make_html=False, report_dir='.', report_level='none'
         # Do aca review checks and update acas[0] in place
         exc = run_aca_review(load_name=f'Obsid {self.obsid}', acars=acars, make_html=make_html,
                              report_dir=report_dir, report_level=report_level,
-                             roll_level=roll_level,
+                             roll_level=roll_level, roll_args=roll_args,
                              loud=False, raise_exc=raise_exc)
         return exc
 
@@ -484,7 +557,7 @@ def review_status(self):
     @property
     def att_targ(self):
         if not hasattr(self, '_att_targ'):
-            self._att_targ = self._calc_targ_from_aca(self.att, 0, 0)
+            self._att_targ = self._calc_targ_from_aca(self.att, *self.target_offset)
         return self._att_targ
 
     @property
@@ -599,6 +672,7 @@ def make_roll_options_report(self):
         roll_context['roll_options_index'] = rolls_index.as_posix()
         for key in ('roll_min', 'roll_max', 'roll_nom'):
             roll_context[key] = f'{self.roll_info[key]:.2f}'
+        roll_context['roll_method'] = self.roll_info['method']
         self.context.update(roll_context)
 
         # Make a separate preview page for the roll options
@@ -632,8 +706,8 @@ def plot(self, ax=None, **kwargs):
         idxs = self.get_candidate_better_stars()
         stars = self.stars[idxs]
         for star in stars:
-            already_checked = ((star['id'] in self.acqs.cand_acqs['id']) and
-                               (star['id'] in self.guides.cand_guides['id']))
+            already_checked = ((star['id'] in self.acqs.cand_acqs['id'])
+                               and (star['id'] in self.guides.cand_guides['id']))
             selected = (star['id'] in set(self.acqs['id']) | set(self.guides['id']))
             if (not already_checked and not selected):
                 circle = Circle((star['row'], star['col']), radius=20,
@@ -797,6 +871,11 @@ def check_guide_geometry(self):
         guide_idxs = np.flatnonzero(ok)
         n_guide = len(guide_idxs)
 
+        if n_guide < 2:
+            msg = f'Cannot check geometry with fewer than 2 guide stars'
+            self.add_message('critical', msg)
+            return
+
         def dist2(g1, g2):
             out = (g1['yang'] - g2['yang']) ** 2 + (g1['zang'] - g2['zang']) ** 2
             return out

sparkles/index_template_preview.html

@@ -91,7 +91,9 @@ <h1> {{load_name}} sparkles review</h1>
 {% if roll_options_table %}
 <h3>Roll options (roll_min={{roll_min}}
         roll_nom={{roll_nom}}
-        roll_max={{roll_max}}</h3>
+        roll_max={{roll_max}}
+        method={{roll_method}}
+      </h3>)
         {{roll_options_table | safe}}
 {% endif %}
 
@@ -118,7 +120,8 @@ <h2> {{id_label}} {{aca.report_id}} at {{aca.date}}</h1>
         <h3><a href="{{aca.context['roll_options_index']}}">Roll options
         report</a> (roll_min={{aca.context['roll_min']}}
         roll_nom={{aca.context['roll_nom']}}
-        roll_max={{aca.context['roll_max']}})</h3>
+        roll_max={{aca.context['roll_max']}}
+        method={{aca.context['roll_method']}})</h3>
         {{aca.context['roll_options_table'] | safe}}
         <span>
       {% endif %}