Improve roll options including uniform sampling of roll range

[taldcroft]

Description

This implements a fall-through option to uniformly sample the entire roll range in cases when the faster algorithm fails to find a solution with no criticals.

It is also improves the original algorithm by including the roll interval corresponding to the original MP-requested roll, but using a roll at the center of the roll interval. The "roll interval" means the full range in roll where the set of candidate stars is unchanged. In the testing below, two of the three obsids were improved because of this change.

A final update is to order the roll options in descending order of improvement, basically putting the best options (from the ACA perspective) first.

Review

• FOT MP review and acceptance of functional test results (@jayhead13 on Slack, June 15 in aspect)
• SOT aspect review of functional test results and code

Testing

A number of observations where identified by FOT MP as problematic during the planning process, requiring some special handling and possibly failing to find a good roll option. Several of these (21486, 21172, 23129, 21867) seemed to be OK now, possibly related to the newer acquisition probability model. However, sparkles 4.6.0 did in fact fail to find a good roll option for three observations, shown below. With this PR, a good option was found in all cases. Obsid 22508 is the one example where the uniform roll option finding was required.

Obsid 21706

• Flight 4.6.0 roll options
• Dev roll options

Obsid 22508

• Flight 4.6.0 roll options
• Dev roll options

Obsid 20922

• Flight 4.6.0 roll options
• Dev roll options

APR0620A

The entire APR0620A schedule was run:
https://cxc.harvard.edu/mta/ASPECT/tests/sparkles/pr106/APR0620A_sparkles/

Test script for individual OR's

The script below was used to generate the test outputs. In all cases this used a dev version of proseco that allows setting the target_offset (sot/proseco#328). This is required for this PR.

import os
import sys
import shutil
from pathlib import Path
from pprint import pprint

sys.path.insert(0, str(Path.home() / 'git' / 'proseco'))

from proseco import get_aca_catalog
import sparkles
from astropy.table import Table
from mica.starcheck import get_starcheck_catalog
from chandra_aca.transform import calc_aca_from_targ
import Ska.Sun

obsid = int(sys.argv[1])

sc = get_starcheck_catalog(obsid)
oflsdir = Path(os.environ['SKA']) / 'data/mpcrit1/mplogs' / sc['mp_dir'][1:]

dynoff = list(oflsdir.glob('*dynamical_offsets.txt'))[0]
dynoffs = Table.read(dynoff, format='ascii')
dynoffs.add_index('obsid')
obs = dynoffs.loc[obsid]

targ_ra, targ_dec = obs['target_ra'], obs['target_dec']
y_off = (obs['target_offset_y'] + obs['aca_offset_y']) / 3600
z_off = (obs['target_offset_z'] + obs['aca_offset_z']) / 3600
targ_roll_nom = Ska.Sun.nominal_roll(targ_ra, targ_dec, obs['mean_date'])
att_aca_nom = calc_aca_from_targ((targ_ra, targ_dec, targ_roll_nom), y_off, z_off)  # nominal roll

aca = get_aca_catalog(obsid=obsid, att=att_aca_nom, target_offset=(y_off, z_off))
acar = aca.get_review_table()

outdir = Path.cwd() / f'roll_{sparkles.__version__}' / str(obsid)
if outdir.exists():
    shutil.rmtree(outdir)
outdir.mkdir(parents=True)

print(f'Running review for obsid={obsid} at {att_aca_nom.equatorial} into {outdir}')
pprint(acar.call_args)

acar.run_aca_review(make_html=True, report_dir=str(outdir), roll_level='critical')

Fixes #138

[jeanconn]

Regarding "It is also improves the original algorithm by including the roll interval corresponding to the original MP-requested roll, but using a roll at the center of the roll interval. " I don't understand this change via this language. I'll try to understand it in context in the code.

[jeanconn]

In the testing, I'm not clear about why you used the observation mean time to set each observation to that mean time nominal roll attitude? I was planning to test on the side via doing something complementary so no big deal either way.

[jeanconn]

Not a request, but I wonder if the min_improvement should be relative to the last "improved" saved option as the space is explored? That could help to keep the saved options from getting too dense.

[taldcroft]

@jeanconn - this is the one line of code that implements "it is also improves the original algorithm by including the roll interval corresponding to the original MP-requested roll, but using a roll at the center of the roll interval." It will probably be much more obvious here in the code, I did struggle trying to describe it and knew that what I wrote was not really adequate.

Basically it generates a list of roll intervals and in the previous code version it was excluding the roll interval which includes the original roll angle. This was a problem for catalogs where there is an acq star very near the FOV edge which is excluded as guide. In this case it would not allow rolling that star into the corner where it would be OK for both acq and guide.

[taldcroft]

In the testing, I'm not clear about why you used the observation mean time to set each observation to that mean time nominal roll attitude? I was planning to test on the side via doing something complementary so no big deal either way.

This was to simulate FOT MP starting off with an observation at the nominal roll. Most of these catalogs are known bad cases where the actual flight version has been moved to a better roll, meaning that sparkles will not try to optimize because the catalog is already fine.

Finding good test cases is not necessarily easy. I thought about taking some marginal catalogs and then cranking up the temperature to force criticals, but at the end figured the 3 that I got from @jayhead13 were enough.

[taldcroft]

Not a request, but I wonder if the min_improvement should be relative to the last "improved" saved option as the space is explored? That could help to keep the saved options from getting too dense.

In practice the "too dense" doesn't seem to happen. I definitely don't want to throw away any viable roll options that can work for FOT MP. The only case that worries me is when doing uniform sampling there might be a roll range that is OK over the whole range and gets sampled at 0.25 deg increments. If that manages to push out another (slightly worse) roll range that would be better for FOT MP then that's bad.

So that makes me think it would be good to expand the max_roll_options default to, say, 20. Or bigger?

@jskrist - is there any limit in the OR viewer API to the number of roll options that can be returned?

[jeanconn]

With regard to "in practice this doesn't seem to happen", that is fair. I was running my testing with the temperature turned up and mostly seeing that, not paying much attention to the order in which these were checked, it looked like it was giving me more options that were worse than ones already in the list:

https://cxc.cfa.harvard.edu/mta/ASPECT/tmp/sparkles_dev/APR0620A_sparkles/obs47322/rolls/index.html

That was also true of

https://cxc.cfa.harvard.edu/mta/ASPECT/tmp/sparkles_dev/APR0119B_sparkles/index.html#id48292

[jeanconn]

"This was to simulate FOT MP starting off with an observation at the nominal roll." I suppose I have no idea if they use the middle of the observation for the approximation of nominal roll. I suppose that is somewhat in the weeds. Your point about shooting to set these up at nominal roll is good; I assume everything is set up so the display still shows the roll options relative to the deviation from the commanded roll and not nominal though (if they have a different reason for picking the roll, still want the closest good choice to that, right)?

[jskrist]

@taldcroft, There is no limit specified in the code, and the dialog will dynamically size based on the size of the message being displayed, which means that the practical limit is based on the height of the end user's screen, as it is not currently set up with a scrollable pane.

[jskrist]

btw, on my screen, that equates to about 32 messages, not roll options, but messages, as each message ('Warning', 'Critical', 'info', etc.) gets it's own line in the output.

[jskrist]

max_roll_options is not mentioned in the documentation comment for roll_args, is this intentional?

[taldcroft]

Fixed.

[taldcroft]

Fixed, see 70ce3f4.

[taldcroft]

@jeanconn - can you give me the commands to reproduce your test results? When I run 47322 with all the temperatures forced to -4.5 C, I get a more reasonable result, see below. In particular it is not falling through to the brute-force method because the "smarter" method finds a good answer.

[taldcroft]

One minor comment that the links you made above in ASPECT/tmp/ will become broken at some point when tmp gets cleaned. Maybe we don't care all that much, but I started putting test results for the record in ASPECT/misc. Maybe we should start of a convention of ASPECT/tests/<package>/ and then just dump functional test results there.

[jeanconn]

Sure Tom. I was being much more casual than you and just looking for weirdness over more catalogs. I used the target offsets without updating attitudes, and I changed temperatures without changing ACA offsets. https://gist.github.com/jeanconn/cc4340bfc5a93fbee27d1848e24ffb04

APR0620A_proseco.pkl.gz

[jeanconn]

I was also wondering if it would have value in in the table to capture which method it was using to get the suggestion? But that seemed more useful for debug than for MP so I think I didn't mention it in the first round of testing.

[taldcroft]

@jeanconn - thanks for the script and great idea for testing this a bit more thoroughly on full schedules. This turned up a bug in the code (fixed in 76a931f). Here is my run for APR0620A:

https://cxc.harvard.edu/mta/ASPECT/tests/sparkles/pr106/APR0620A_sparkles/

This reflects a new limit of 20 roll options (increased from 10, ac62ec6) and a change in the default roll delta from 0.25 deg to 0.5 deg for uniform sampling (5f8267f).

To do: re-run the other functional test cases in particular 22508.

[taldcroft]

Looking at the results for APR0620, there were no cases where the uniform roll option actually came up with a good catalog (where the uniq_ids method had failed). Right now 22508 is the only known case where uniform makes a difference.

I'm thinking about changing the default so that it does not fall through to uniform for ERs. I'm worried about this being slow for ER's given that most of the time it won't help and one could just pick a new attitude. OR's are different because there is no choice about target RA, Dec.

[jeanconn]

"I'm thinking about changing the default so that it does not fall through to uniform for ERs.": I like this.

I had been wondering about short-circuiting earlier on ERs: as in if you only have poor possible stars in the field, rolling to see if you get the best P2 fewest warnings based on distance from edge and dark map seems not a great use of time. Your idea to just not fall through on ERs is sounding simpler and better than other ways to approach this.

[taldcroft]

OK, I made the update and also added the roll option method to the web output. (It was already in the roll info object as method).

[jeanconn]

The changes after my initial review look to have addressed my feedback. So it looks like actions are to rerun the functional test cases? Also, from a reread of the thread it looks like 20 possible roll candidates might still be too many for the 32 lines in the matlab tools?

[taldcroft]

I re-ran functional tests and confirm the outputs are still good. Unit tests are passing on Mac. The default max_roll_options was set back to 10. Merging this to avoid letting it get stale again.