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

Add calc_spoiler_impact function and use instead of box filter for acq #165

Merged
merged 8 commits into from
Oct 5, 2018

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Oct 5, 2018

This computes the centroid offset and fractional change in summed counts caused by spoilers.
It runs in about 1 msec for a single spoiler.

To do:

  • Unit tests with synthetic values to demonstrate expected behavior
  • Add 21068 functional test
  • Fix current breaking test (include_exclude).
  • Code comments

@taldcroft
Copy link
Member Author

The basic idea is the same as for guide, details slightly different.

@jeanconn
Copy link
Contributor

jeanconn commented Oct 5, 2018

Interesting. I had played with using the fm centroid, but didn't have this idea of using a mean high background estimate.

There are the couple of edge cases that I think of wrt to this approach... that multiple spoilers can "balance out" (should be rare and we don't care for acquisition) or that the spoiling star right on the edge of the 6x6 ends up coming more onto that pixel than modeled here. For acquisition I didn't know if that second case might be an issue, because I thought the placement of the 8x8 is hits-based not coordinate based.

But probably all in the noise.

@taldcroft
Copy link
Member Author

@jeanconn - good points. I had been making a validation notebook for this function anyway, and I think that your concerns are addressed now. The notebook might be useful if you want to play around and decide to use this for guide.

http://nbviewer.jupyter.org/url/asc.harvard.edu/mta/ASPECT/proseco/spoiler-impact.ipynb

On the "balancing out", I put in code to insist that if there are multiple spoilers, they all get stacked in the same quadrant of the image readout window.

On the issue of critical behavior near a pixel boundary, the plots in the notebook show that there does not appear to be very strong pixel-related changes in the output values. There is some discernible effect, but not strong. Also, in practice what is going to cause a rejection is the frac_norm value, not centroids. There is margin in the rejection threshold, so if the actual frac_norm on-orbit is a bit lower, not a problem.

@taldcroft taldcroft changed the title WIP: add calc_spoiler_impact function Add calc_spoiler_impact function and use instead of box filter for acq Oct 5, 2018
@jeanconn
Copy link
Contributor

jeanconn commented Oct 5, 2018

On the issue of critical behavior near a pixel boundary, the plots in the notebook show that there does not appear to be very strong pixel-related changes in the output values.

Gotcha. There's not much text there to hold my hand through what you've done to validate that, so it is going to take me longer to understand.

@taldcroft
Copy link
Member Author

If you look at the images showing centroid offsets, they are relatively smoothly varying and you don't see strong square-like structures corresponding to pixel boundaries. That's all I meant.

@taldcroft
Copy link
Member Author

To be clear you can see evidence of the pixels, but not like a wall somewhere.

@taldcroft
Copy link
Member Author

For use with acq selection, I think this is good and I'm merging.

@taldcroft taldcroft merged commit eb9ec2e into master Oct 5, 2018
@taldcroft taldcroft deleted the spoil branch October 5, 2018 17:12
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