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

Confirm maximum extent of CCD allowed for acq stars. #17

Closed
jeanconn opened this issue Jul 20, 2018 · 14 comments
Closed

Confirm maximum extent of CCD allowed for acq stars. #17

jeanconn opened this issue Jul 20, 2018 · 14 comments

Comments

@jeanconn
Copy link
Contributor

Confirm maximum extent of CCD allowed for acq stars.

Starcheck's "Angle Too Large" check is


   my $ccd_row_min = -512.5;
    my $ccd_row_max =  511.5;
    my $ccd_col_min = -512.5;
    my $ccd_col_max =  511.5;

    my $pix_window_pad = 7; # half image size + point uncertainty + ? + 1 pixel of margin                   
    my $pix_row_pad = 8; # min pad at row limits (pixels) [ACA requirement]                                 
    my $pix_col_pad = 1; # min pad at col limits (pixels) [because outer col is not full-sized]             

    my $row_min = $ccd_row_min + ($pix_row_pad + $pix_window_pad);
    my $row_max = $ccd_row_max - ($pix_row_pad + $pix_window_pad);
    my $col_min = $ccd_col_min + ($pix_col_pad + $pix_window_pad);
    my $col_max = $ccd_col_max - ($pix_col_pad + $pix_window_pad);

...

if (   $pixel_row > $row_max - $pix_slot_dither || $pixel_row < $row_min + $pix_slot_dither
                   || $pixel_col > $col_max - $pix_slot_dither || $pixel_col < $col_min + $pix_slot_dither)\
 {
                push @warn,sprintf "$alarm [%2d] Angle Too Large.\n",$i;
            }

Starcheck has the known issue that lack of dither amplitude continuity can mean that pix_slot_dither is incorrectly the 20 arcsec version (4 pix) not the 8 arcsec version (1.6 pix). Besides that, the window pad seem reasonable to me but perhaps could come down to just half the image size. I don't recall the source document for the 8 pixel row pad.

@jeanconn
Copy link
Contributor Author

It might make sense to add (subtract?) the window and dither pads to proseco for proseco version 1.0 and consider doing the analysis to reduce the pads at some point in the future. Seems like this would allow a safer expedited promotion strategy.

@taldcroft
Copy link
Member

Thanks for checking in to this. This is handled in two places:

def calc_p_in_box(row, col, box_sizes):

max_ccd_row = 512 - 8 # Max allowed row for stars (SOURCE?)

Starcheck and SAUSAGE take the approach of defining limits to guarantee ~100% chance of the star being on the CCD in a position that can be successfully read out. Proseco instead allows the star centroid to be anywhere up to exactly the edge of "usable" part of the CCD, and then models the probability it will end up failing because of being outside the usable part of the CCD. (In fact there is no real reason to not allow choosing bright stars that are nominally slightly off the CCD in hopes that the maneuver error will bring it on).

Proseco tries to avoid the concept of "pads". One models various factors (dither, maneuver error) that impact the probability distribution for where the star lands along with the size of the readout box for getting a good star image. As mentioned in SSAWG, the current code is not quite right, but that will be fixed.

With proseco star selection we'll probably want to downgrade the current Angle too large to green, and possibly add a new orange warning for stars that are really way outside (threshold TBD) the CCD limits.

@jeanconn
Copy link
Contributor Author

jeanconn commented Jul 23, 2018

Yes, I saw from the characteristics that you were subtracting the "row pad" of 8 pixels even if we aren't calling it a pad at this point.

I also fully support not doing anything for adjusting the available area of the CCD based on maneuver error.

For the "Angle Too Large" warning I was addressing, as far as I see and can tell, starcheck is not considering maneuver error in this determination. It is only confirming that the star is on the useable part of the CCD based on a window size pad, a dither pad, and the ccd bounds (which appear to be different by half pixels from yours but I'm not sure if that is just the pixel coordinate reference difference). SAUSAGE does try to verify the 100% chance of the star and the box being on the CCD but starcheck here is just trying to make sure the 8x8 box centered on the centroid will be able to get the light from the star to have a reasonable position to for the centroid.

I just figure that the current proseco approach of allowing the centroid (calculated at exactly the target attitude) to be right up to the usable edge means that we may only have part of the star on the CCD (reduced magnitude or no track). For a star right at the CCD edge (as calculated by the centered position at the target attitude), but with 20 arcsec dither, doesn't that leave us with approximately a 25% chance that no star light from the star will be on the CCD? Where is that captured in the probabilities?

@taldcroft
Copy link
Member

Just to be clear, the current code is wrong, so the real discussion of this should take place on the PR that fixes it, and possibly as a starcheck issue.

As an edge case to ponder (har har), if you have a star that is exactly at the usable CCD edge, with zero maneuver error and zero dither, then that places half of the 8x8 readout box off the usable CCD. In this case the acquisition probability should be zero because (based on our rules) the readout will fail. In reality the PEA does a larger readout, I think with one additional leading row, so that might imply considering a 10 rows x 8 cols box for sizing. Then one computes the fraction of the area allowed by dither and maneuver error in which the 10x8 readout box is entirely within the usable CCD.

So I think we are agreeing.

@jeanconn
Copy link
Contributor Author

So I think we are agreeing.

I'm not entirely following "Then one computes the fraction of the area allowed by dither and maneuver error in which the 10x8 readout box is entirely within the usable CCD." but will plan to come back to this after the next PR. Thanks!

@taldcroft
Copy link
Member

I believe this is closed by #18. @jeanconn?

@jeanconn
Copy link
Contributor Author

Basically, I was hoping that this would converge so that in end I wouldn't see the starcheck "Angle Too Large" warnings on proseco.acq catalogs. Did you rerun those catalogs and how do they look?

@taldcroft
Copy link
Member

(oops, commented in wrong issue).

Proseco will select a bright star that is nominally off the CCD if p_acq is bigger than other available stars. Basically you can have a roughly 50/50 that the maneuver error will bring it on to the CCD.

So I think the starcheck Angle Too Large check needs modification, maybe just turned into a yellow warning for acq. It could be orange if the star is more than 60 arcsec + dither off the CCD, which means that proseco has a bug because then p_acq should really be very close to zero.

@taldcroft
Copy link
Member

@jeanconn - can we close this?

@jeanconn
Copy link
Contributor Author

My thought was that you'd rerun the previous test loads and then we'd change starcheck to agree with you on the the edge cases. Do you want to capture that somewhere?

@jeanconn
Copy link
Contributor Author

Basically, I don't want to have anything for which we say "we just ignore that red warning in starcheck now" without a bit more review and then changing the warning in starcheck.

@taldcroft
Copy link
Member

Changing starcheck to work with proseco acq stars is (at a high level) captured in sot/starcheck#252, and the issue of Angle too large is specifically mentioned.

I want to close this proseco issue because it pertains the question of whether proseco is handling the CCD extent correctly, which I think it is.

@jeanconn
Copy link
Contributor Author

OK. I'll review. I thought you'd said it wasn't handling the edge boxes correctly before, and you haven't run the starcheck outputs, so I'm not sure which test cases confirm that edge boxes are correct.

@taldcroft
Copy link
Member

It wasn't and then I fixed it in #18. At least that's my understanding.

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

No branches or pull requests

2 participants