search

c3-time-domain / SeeChange

A time-domain data reduction pipeline (e.g., for handling images->lightcurves) for surveys like DECam and LS4
BSD 3-Clause "New" or "Revised" License
0 stars 4 forks source link

Calibration file refactor #162

Open guynir42 opened 11 months ago

guynir42 commented 11 months ago

This is not actionable just yet. We need to think and then discuss if and how we want to do this.

I was going over the Instrument/DECam calibration loading code. I was going to implement something similar for PTF to use in the test fixtures but in the end decided to just manually download the bad pixel map (all I needed) and apply it after preprocessing is done. This is fine for testing with PTF images but of course can't work in production. The fact that the current framework was a little too complicated for me is worrying.

It seems like it may be possible to simplify things. Here are some things I think we should consider: 1) Why save some calibration files as DataFile and some as Image? I don't see that saving a flat as an Image object necessarily simplifies things. E.g., we end up having to manually fill out the RA/Dec and corners, and deliberately not use the save() method when saving it to disk. Why not just have all the calibrations in a CalibrationFile object that inherits from FileOnDiskMixin and then have a few types (e.g., spectra, image, whatever)?

2) Not sure why we need the 'externally_supplied' and other 'calibration set' types. I am worried that we have calibration sets and calibration types and it gets confusing. Can't we just agree that each instrument will have a dedicated function to fetch the calibrators from file or from url or from database images (and re-save them as CalibratorFile) and then the calibrator is always an "externally supplied" in that sense?

It may be worth while to simplify as much as we can, and write some documentation on how to provide calibrations, while keeping in mind how we would want this to happen for LS4. That should help clear our vision for how to make the process of adding calibrators as painless as possible (hopefully).

rknop commented 4 months ago

I agree that we should just make calibratorfiles point to a DataFile. I will make that change. (I'm stuck on the conductor right now because the NOIRLab data archive is down.) (Oh, wait, it may be back up!)

Re: calibration set vs calibration type. Calibration type is things like "flat", "linearity", etc.

Calibration set is because we may have different sets of calibrations for the same image. What I've called "externally supplied" are the instrument default ones, but for the same instrument we might also want to build (say) nightly dome, or nightly night-sky flats. They could have overlapping ranges of validity, so the calibration set is a way of distinguishing between them. We may not be in a situation where we have "one true flat" (the way I've assumed for DECam up to now).

Thinking ahead, something we may need to add is a flat builder pipeline. We will not want to do this nightly, because we don't want to wait for the night to be over before processing the night's data. But, we might do it (say) weekly, or monthly. (When I defined the current calibrator set types, I was thinking we would do it nightly.)

rknop commented 4 months ago

As I started to work on this, I decided that this probably shouldn't be done after all.

The reasoning behind getting rid of an Image tie-in to CalibratorFile is that there are a whole bunch of Image properties that don't apply to Calibrator Files (e.g. all the coordinates on the sky stuff).

However, as I was making the edits, I was realizing that in order have CalibratorFile only point at DataFile, and never point at Image, I had to do things like add FITS reading to the DataFile class. But, then, looking at the DECam linearity correction, where the linearity file is a DataFile but of format FITS, simple FITS reading as a 2-d image isn't enough, becuase the linearity file is a multi-HDU file of FITS tables. WE dont' want to just read the entire table in ( waste of memory, you will generally only need one HDU), so a simple "data" property for DataFile wouldn't be sufficient.

For now, I'm going to leave this as it is. The cailbrator data files we're using are mostly FITS images, and even though we don't need all of the infrastructure from the Image class, we do need some of it, and by keeping things as is, we don't have to make new implementations. The only other current use of DataFile is the DEC am linearity files, which has a custom FITS format layout, and for that one format we have right now, it's not worth going through and trying to come up with a generic DataFile reader.