Fix bug that was only using row dither for hot pixel region check #195

jeanconn · 2018-12-14T14:00:54Z

Fix bug that was only using row dither for hot pixel region check

get_ax_range was ignoring the supplied value of "extent" and using row_extent always. This is an issue for observations with asymmetric dither.

get_ax_range is still ugly, but I've moved it to top level so I can use it equivalently from the guide reporting.

Closes #194

proseco/guide.py

taldcroft · 2018-12-14T14:18:51Z

This is related to #125 (where I consider imposters to be a type of spoiler). Unit test with a synthetic star catalog and synthetic dark current is needed. Probably worth adding a quick unit test of get_ax_range as well.

taldcroft · 2018-12-14T14:04:03Z

proseco/guide.py

+    minus = int(np.floor(n - extent))
+    plus = int(np.ceil(n + extent))
+    if (np.floor(n) != np.ceil(n)):
+        if n - np.floor(n) > .5:


Never write .5 always 0.5.

taldcroft · 2018-12-14T14:04:19Z

proseco/guide.py

+    """
+    Given a float pixel row or col value and an integer pixel "extent",
+    return a range for the row or col that is divisible by 2 and contains
+    at least the requested extent


Period at the end and newline.

taldcroft · 2018-12-14T14:14:00Z

proseco/guide.py

+    """
+    minus = int(np.floor(n - extent))
+    plus = int(np.ceil(n + extent))
+    if (np.floor(n) != np.ceil(n)):


I don't understand this logic. Basically you want plus and minus to be even, right? So would this work?

# Ensure plus and minus are both even, expanding the range as needed. if minus % 2 != 0: minus -= 1 if plus % 2 != 0: plus += 1

We want (plus - minus) to be even. If you ensure that both are even (or odd) you do win that, but you can end up adding extra pixels to both ends that really aren't going to be in the 8x8.

With the logic that is in there now, the initial

minus = int(np.floor(n - extent)) plus = int(np.ceil(n + extent))

guarantees you have a odd range (for real float values not exactly at the pixel boundary), and then it adds one more pixel to the side closest to the offset of the star within the center pixel (IIRC).

If this is about figuring out the readout window then can we use the usual logic for defining a readout window?

n_int = int(round(n)) # for `n` in edge pixel coords plus = n_int + extent minus = n_int - extent

The extent is a float value. If you want to round it up, this works too, but will get extra pixels.

Where's the usual logic for defining a readout window? If I should always be taking one off the minus or adding one to the plus anyway, that's fine, but practically, I was trying not to add many extra pixels while still making it possible to bin 2x2. If that's just too ugly we can do something different, but may end up with regression test changes.

Here's one implementation of the "usual logic":
https://github.com/sot/chandra_aca/blob/master/chandra_aca/aca_image.py#L451
I'm sure you find it in annie as well.

I don't quite follow what you are saying, but the 3 lines I wrote will always give the minimal set of pixels that are centered on n and can be binned 2x2. I suspect it is exactly equivalent to what you wrote.

I'm still confused. extent is meant to have 4 pixels for the 8x8 image and, generally, plus 1.6 pix for 8x8 dither. So plus = n_int + 5.6 doesn't quite work, but if I round up in both directions again that probably works.

So, for a star with nominal row=0.7 in edge coordinates, what should the dithered pixel region be in row?

-6 to 6
or
-5 to 7

Because I thought -5 to 7 was the right answer, but I didn't have the usual logic as my reference.

taldcroft · 2018-12-14T16:50:13Z

The docstring said extent is an integer, and that does make a difference here. I was predicating my comments on that assumption. For non-integer values it might be a different story.

jeanconn · 2018-12-14T16:53:17Z

Oops. Thanks Tom. Introduced docstring bug fixed.

taldcroft · 2018-12-15T10:51:29Z

proseco/tests/test_guide.py

+
+
+def test_get_ax_range():
+    # Confirm that the ranges from get_ax_range are reasonable for a variety of


Use docstrings for tests so they show up in API docs.

taldcroft · 2018-12-15T10:51:33Z

proseco/tests/test_guide.py

+
+def test_edge_star():
+    """
+    Add stars right at row and col max for 8" dither.


This test is good but it seems like it does not actually test the bug fix related to get_ax_range. That would happen with hot pixel imposters right?

Also in both this test and a synthetic hot pixel test, document the exact source of the magic numbers (e.g. 495.3). I'm never sure whether it is best to have test values like this hardcoded, so it gives a visible regression if characteristics change, or have them computed in the test so tests continue passing. If you don't care then I'd say the latter.

row_max = CCD['row_max'] - (CCD['row_pad'] + CCD['window_pad'] + r_dith_pad) col_max = CCD['col_max'] - (CCD['col_pad'] + CCD['window_pad'] + c_dith_pad)

Also in both tests it would be best to use asymmetric dither, esp for imposters since that was the original point of the bug fix.

Yeah, I was working on a new test of hot pix imposters Friday but didn't finish. Will get it in. And sure, documenting exact source of the numbers sounds good (I at least left a note about nominal dither being required for the test setup, but can also recalculate it directly here. Figured it was unlikely we'd change CCD characteristics without needing to update all the tests anyway.). For the edge case test, I don't think it matters if the test is done with asymmetric dither or not, but fine to do so. For my specific bug also need to test with the "large" dither in each axis.

proseco/guide.py

jeanconn · 2018-12-17T18:10:20Z

I converted "n" to "rc" in get_ax_range, but also didn't like that the use of "ceil" in the way that get_ax_range was being called effectively meant that ACIS observations got a 0.4 arcsec pad, but HRC observations got no padding. So I added a pad to each and got rid of that use of ceil. Was that too much @taldcroft ?

taldcroft · 2018-12-17T18:30:42Z

proseco/characteristics_guide.py

+dither_pix_pad = 0.4
+
+# Minimum imposter mag (if dark current is 0 set explicitly to this value)
+min_imposter_mag = 20.0


This is actually the max value of imposter_mag. Otherwise this and the code that uses it are OK.

Hah. I should have used a variant on faint.

How about "imposter_mag_for_zero_dark"?

Fine. Plan B would be to replace pixmax with np.clip(pixmax, 1.0, None) in the offending line, add a code comment, and be done with it. I somehow feel like characteristics should be reserved for constants that have a discernible algorithmic impact. But that is my style and not necessarily correct. Your choice. FYI, count rate of 1.0 is very nearly 20th mag. 😄.

Sure. I like that better. And yes, I saw that count rate of 1 is basically 20th mag. I considered just setting pixmax to 1.0 before starting the loop over the sums, but didn't want to just add 1.0 in general. Your clip is better.

jeanconn · 2018-12-17T19:56:51Z

OK, so maybe this is now done? I didn't see feedback on the 0.4 pad I added to (the HRC) dither boxes.

I did not update version figuring that could go in after merging (and after any other updates).

Fix bug that was only using row dither for hot pixel region check

23e7982

jeanconn commented Dec 14, 2018

View reviewed changes

proseco/guide.py Show resolved Hide resolved

jeanconn requested a review from taldcroft December 14, 2018 14:01

taldcroft reviewed Dec 14, 2018

View reviewed changes

jeanconn added 4 commits December 14, 2018 09:30

Update comment, docstring, number format

93a51ef

Simplify get_ax_range a bit

4f79452

Update docstring again in get_ax_range

c824749

Add test of 'reasonableness' of get_ax_range

6d7a721

Fix get_ax_range docstring (extent not integer)

97c6cb9

Add test with as-close-to-edge as possible guide stars

57d0bb7

taldcroft requested changes Dec 15, 2018

View reviewed changes

jeanconn added 5 commits December 16, 2018 18:50

Add hot pixel + dither tests

9ec89d8

Add more dither test cases to edge star selection test

3b87d7d

Convert test comment to docstring

5649118

Rename 'n' to 'rc' in get_ax_range

67de0a8

Add small pad to pixel region

9185a7e

jeanconn mentioned this pull request Dec 17, 2018

Guide star selection tracks imposter mag, but is 'inf' for 0 dark current #196

Closed

taldcroft reviewed Dec 17, 2018

View reviewed changes

jeanconn force-pushed the extent branch from d5ce279 to 7b67735 Compare December 17, 2018 18:37

Clip pixmax so it is never 0

737225a

jeanconn force-pushed the extent branch from 7b67735 to 737225a Compare December 17, 2018 19:32

taldcroft approved these changes Dec 18, 2018

View reviewed changes

jeanconn merged commit 74e94eb into master Dec 18, 2018

jeanconn deleted the extent branch December 18, 2018 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug that was only using row dither for hot pixel region check #195

Fix bug that was only using row dither for hot pixel region check #195

jeanconn commented Dec 14, 2018 •

edited

Loading

taldcroft commented Dec 14, 2018

taldcroft Dec 14, 2018

taldcroft Dec 14, 2018

taldcroft Dec 14, 2018

jeanconn Dec 14, 2018

jeanconn Dec 14, 2018 •

edited

Loading

taldcroft Dec 14, 2018

jeanconn Dec 14, 2018

jeanconn Dec 14, 2018 •

edited

Loading

taldcroft Dec 14, 2018

jeanconn Dec 14, 2018 •

edited

Loading

taldcroft commented Dec 14, 2018

jeanconn commented Dec 14, 2018 •

edited

Loading

taldcroft Dec 15, 2018

taldcroft Dec 15, 2018

jeanconn Dec 15, 2018

jeanconn commented Dec 17, 2018

taldcroft Dec 17, 2018

jeanconn Dec 17, 2018

jeanconn Dec 17, 2018

taldcroft Dec 17, 2018

jeanconn Dec 17, 2018

jeanconn commented Dec 17, 2018



		def test_get_ax_range():
		# Confirm that the ranges from get_ax_range are reasonable for a variety of

Fix bug that was only using row dither for hot pixel region check #195

Fix bug that was only using row dither for hot pixel region check #195

Conversation

jeanconn commented Dec 14, 2018 • edited Loading

taldcroft commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

taldcroft commented Dec 14, 2018

jeanconn commented Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn commented Dec 17, 2018

jeanconn commented Dec 14, 2018 •

edited

Loading

jeanconn Dec 14, 2018 •

edited

Loading

jeanconn Dec 14, 2018 •

edited

Loading

jeanconn Dec 14, 2018 •

edited

Loading

jeanconn commented Dec 14, 2018 •

edited

Loading