Remove really bright stars from early stages and modify offset thresholds

[jeanconn]

Remove really bright stars from early stages and modify offset thresholds.

[jeanconn]

I figure we have no time to test this, but just thought I'd float the idea.

[jeanconn]

This was a quick-n-dirty approach if we decided to rush it for the quick release. It might also be the most straightforward way to go for a 4.4 release, but I'm not sure if we want to do anything with setting a lower bound on mag_err for bright stars (as a code change).

Additionally, the current stages set the sigma multiplier to zero in the last two stages; it seems to me that might not be prudent in general (which I think would still allow a 5.9 mag star with mag_err 0.99 to be selected in the last stage). Changing the sigma multiplier I think could change too many catalogs (not by much, but probably not worth evaluating in detail). Thus it could make sense to set the bright limit to 6.8 (5.8 plus max allowed mag_err of 1.0) in the last two stages. That would still keep scope of the change isolated to bright stars.

[taldcroft]

See the notebook proseco/mag-err-bright-stars-proseco.ipynb.

[taldcroft]

From the analysis I did, I think this PR appears reasonable. The action now is to do some moderately large-scale validation to assess the impact, which is basically unknowable a-priori at least to my brain.

[jeanconn]

Thanks! After I finish with my first round review of the bright star data (with your notebook etc), I'll also see if we may want to just set SigmaMultiplier to 1 in the last stages. Will review impact of all the choices on a hopefully-sufficiently-large validation set.

[jeanconn]

I also figure that there's really no big issue in just bounding the effective mag_err itself for this specific check. So when combined with the stages and the SigErrMultipliers, this is effectively:

Stage 1: limit 6.2, multiplier 3, brightest possible star 6.5
Stage 2: limit 6.2, multiplier 2, brightest possible star 6.4
Stage 3: limit 6.1, multiplier 1, brightest possible star 6.2
Stage 4: limit 6.0, multiplier 1, brightest possible star 6.1
Stage 5: limit 5.9, multiplier 1, brightest possible star 6.0
Stage 6: limit 5.6, multiplier 1, brightest possible star 5.8 (the 2 multiplier applies at extreme end so this is 2 * 0.1 + 5.6

[taldcroft]

I read through the comments and understand most of the words but to be honest I have no insight into what is really happening. So from the perspective of "how well should this work" you are mostly flying solo. The validation will need to be carefully thought through and presented.

[jeanconn]

Understood. Though part of the problem is that "how well should this work" validation will just show that my changes effect the change I've intended, not that the change is reasonable! For example, I'll be able to show that there are fewer aca_preview errors, but not that the catalogs are "better". Selected catalogs will be "worse" via the guide_count metric (which knows nothing about error) as bright stars will be skipped.

I could make this more complicated by penalizing bright stars more when selecting a final catalog, but I think that doesn't really win us much.

I am also considering adding more error on this bounds check to stars with color=0.7 that can be selected in the last stage. proseco.core sets the lower bound on mag_err to 0.3 for color=1.5 stars but there's nothing for color=0.7 stars.

[jeanconn]

In validation what I see so far is, as expected, these bright stars are either bumped to later stages or not selected at all. Comparing aca_preview validation warnings as a first go test, this means a small number of ERs no longer have enough bright stars. This was also expected and I think that is acceptable and is not worth separate handling of ERs (we could for example allow these bright stars for ERs but not for ORs).

[taldcroft]

@jeanconn - excellent!

[jeanconn]

Not sure how big a validation set to use for final sign-off, and still want to look at how much of a hit we see in guide_count overall.

[taldcroft]

6 months is probably fine. For the first 19 years we were basically selecting right down to 5.8, right? So it's good to recognize this issue and do better when possible, but it isn't a critical from a safing perspective. At some level we just have to ask for the 1 in 50 chance where it is too bright, is the rest of the catalog good enough?

[jeanconn]

It looks like this code moves (~1 in 20) or removes (~a separate 1 in 20) a bright star in 1 in 10 catalogs. For the ones where it has been removed and the catalog isn't good enough via guide count, I had separately wondered about extending the "try a different combination of candidates" code, but will plan to finish up this PR and validation and look at that separately if there's time.

[jeanconn]

I made more changes at once than you will probably like in ea4fa05 @taldcroft , but I think fundamentally when we are playing with the stages it doesn't make that much sense to test independently anyway.

If you would prefer that I just bump the stage 6 to 5.8 I can go that way too... I just figured that the offsets are big effect (that plot of mag vs stage really helped me see that) and we are looking to cap them a bit anyway.

[taldcroft]

Looks good!

[jeanconn]

Setting to 5.6 is a hack that should allow stars up to 2 * 0.1 away to be selected (5.8 brightest). Not sure where to put that in comments in the config file.

[taldcroft]

Why doesn't the SigErrMultiplier apply here?

Putting the comment at this line in code seems fine. I would also add a comment at the top of the stages referencing some TWiki page or section where you document all the notebooks and analysis that bears on these params.

[jeanconn]

Regarding SigErrMultiplier, that's due to a bunch of compromises here when I thought we didn't want to let scope creep including the hacks at:

proseco/proseco/guide.py

Line 207 in ea4fa05

((cand_guides['mag'] - max(n_sigma, 1) * mag_err) < bright_lim) |

and

proseco/proseco/guide.py

Line 209 in ea4fa05

((cand_guides['mag'] - 2 * mag_err) < min(bright_lim, 5.8))

But it might make more sense to get rid of half of those compromises and not care if SigErrMultiplier is 1 for all of the later stages (which can change the spoiler status).

[jeanconn]

Ugh, though if we just set SigErrMultiplier to 1 without other changes, there's also the issue that we end up with significant catalog diffs due to it not selecting faint stars (for example a 10.25 mag star with mag_err 0.06 now could be more than 1 mag_err fainter than the 10.3 limit of the stage...).

Or could just try to redo this so it makes sense.

[jeanconn]

For this I'm rerunning validation with the new stages and will then review (and may tweak thresholds again based on that review). Then I think this will just need some updated tests and improved comments.

[jeanconn]

From a review of the observations that had "available" bright stars between 5.8 and 6.0, it looks like these modified centroid offset thresholds still don't bring in those stars even with t_ccd_guide of 0. I think the offset thresholds could actually be more restrictive, like stage 4 = 1.5, stage 5 = 1.75 , but this is a bigger calibration effort, so my thought is to go with these PR suggestions for now.

[jeanconn]

Oops, had this fix local but neglected to push before your approval @taldcroft .

[jeanconn]

Updated validation notebook:

https://nbviewer.jupyter.org/url/cxc.harvard.edu/mta/ASPECT/proseco/validation-4.4/bright_star_PR_validation_v2.ipynb

[jeanconn]

And some review of just the change to the imposter thresholds of the stages

https://nbviewer.jupyter.org/url/cxc.harvard.edu/mta/ASPECT/proseco/validation-4.4/bright_star_with_offsets_PR_validation.ipynb

All looks good to and expected to me.

[taldcroft]

Was this passing tests for you? I am seeing a fail now in master on test_dense_star_field_regress that appears to be from this PR. I'm not surprised that changing the bright selection would affect this catalog, but I normally expect that you would indicate if tests weren't passing.

[taldcroft]

This also breaks test_imposters_on_guide in sparkles.

[taldcroft]

To be clear, it doesn't appear there are any problems in the code, just tests that need adjustment because of different star selection.

[jeanconn]

I thought tests were passing at some point when I rebased off 4.4 but will clean up. I had also expected to actually add some tests for this PR and #258 but will see how time goes.