Add mod to be able to get 8x8 images in aca_l0.get_slot_data #247
Conversation
|
IIRC it was already returning 8x8 for the IMGRAW ? So the new flag is more about centering the 6x6 and 4x4 instead of plopping those in the corner of the 8x8? |
|
I'm also a little puzzled about why this is necessary. It seems like the convention that the image data are always in the lower-left part of an 8x8 array is fine. In any case, if this change is required then you would need another field in the record to specify whether in the images are lower-left or centered. |
Nope. It was returning a flat array.
The magnitude calculation is vectorized (see it here). By aligning all images, I do not have to change the pixels that get added depending on the image size. I figured this would be nice to have in the interface. I know it is not the convention used for 20 years, but it is useful. |
Right. But at least it was always 64 (a representation of an 8x8)? For me my question was less about justifying your new change and more about naming the option and describing what it does. |
|
I am happy to use any name you want if you just tell me exactly what you think it should be. When I did it, one option returned images in (64,) the other in (8,8), so to me it sounded natural, but it can be changed. |
mica/archive/aca_l0.py
Outdated
| @@ -42,6 +42,25 @@ | |||
| 'arc5gl_query': 'ACA0', | |||
| 'fileglob': '*fits.gz'} | |||
|
|
|||
| ACA_DTYPE_2 = (('TIME', '>f8'), ('QUALITY', '>i4'), ('MJF', '>i4'), | |||
There was a problem hiding this comment.
Change to
ACA_DTYPE_8x8 = tuple((dt[0], dt[1], (8, 8)) if dt[0] == 'IMGRAW' else dt
for dt in ACA_DTYPE)
(assuming the only change is in the IMGRAW dtype.
There was a problem hiding this comment.
yes, only IMGRAW changed.
mica/archive/aca_l0.py
Outdated
| @@ -95,8 +114,21 @@ def get_options(): | |||
| return opt | |||
|
|
|||
|
|
|||
| def _mask(n): | |||
There was a problem hiding this comment.
This doesn't appear in this file. Orphaned?
There was a problem hiding this comment.
yes, I see it is not used. I must have used it at the beginning. In the end, this mask does not enter in the magnitude calculation, so it is not used anywhere. I will remove it.
mica/archive/aca_l0.py
Outdated
| @@ -135,7 +167,10 @@ def get_slot_data(start, stop, slot, imgsize=None, | |||
| data_files = _get_file_records(start, stop, slots=[slot], | |||
| imgsize=imgsize, db=db, | |||
| data_root=data_root) | |||
| dtype = [k for k in ACA_DTYPE if k[0] in columns] | |||
| if img_shape_8x8: | |||
There was a problem hiding this comment.
Better to not repeat code, so
aca_dtype = ACA_DTYPE_8x8 if centered_8x8 else ACA_DTYPE
dtype = [k for k in aca_dtype if k[0] in columns]
Could also be
dtype = [k for k in (ACA_DTYPE_8x8 if centered_8x8 else ACA_DTYPE) if k[0] in columns]
mica/archive/aca_l0.py
Outdated
| f_imgsize, f_imgsize)) | ||
| if img_shape_8x8: | ||
| if f_imgsize == 8: | ||
| all_rows['IMGRAW'][rowcount:(rowcount + len(chunk))] = chunk.field('IMGRAW') |
There was a problem hiding this comment.
As long as you are in here, maybe fix this many times repetition of rowcount:(rowcount + len(chunk)). Define idx0 and idx1 up front and use it everywhere. And isn't chunk[name] the same as chunk.field(name) these days?
There was a problem hiding this comment.
Will do. Indeed, I just read a fits file and chunk[name] the same as chunk.field(name). No idea what it was in the old days 😃
mica/archive/aca_l0.py
Outdated
| def get_slot_data(start, stop, slot, imgsize=None, | ||
| db=None, data_root=None, columns=None, | ||
| img_shape_8x8=False |
|
I agree that centered on 8x8 feels better to me, but historically deferred to Jean's original design decision there. This is indeed very old code. But let's do this, I just put in some comments. We could quibble about your "vectorized" calculation since that still calls a numba function that does the staggering of the dark cal. Basically the reason I never cared too much about centered or not is that you always have to do some sort of per-image alignment to get into a fixed reference frame of ACA pixels on the CCD. In other words I never had a need to compress an N x 8x8 stack of L0 images down to 8x8 (i.e. mean on axis 0). |
…ument to centered_8x8, removed orphan _mask function, modified dtype definition inside get_slot_data. Other simplifications.
|
I made changes according to Tom's comments. |
|
The code looks good to me now as far as I can tell, with the exception of a couple of This needs a test which covers the new code. Probably just extending the obsid 8008 test and also doing the same centroid comparison for an 8x8 fid light in that obsid would be sufficient. |
I just added that test. |
Which ones are you talking about? this one?. If so, these are not the only whitespaces I would change... |
javierggt
changed the title
Description
This PR adds the posibility to get the images with 8x8 shape from l0. This is used in the magnitude estimation algorithm.
Testing