Paul do you have time for a review? Prefer PRs?

Few notes:

• This ticket first and foremost edits warpAndPsfMatchTask and MakeCoaddTempExp to simultaneously make direct and psfMatched CoaddTempExps, in order to only warp once.

• I also changed assembleCoadd to make both direct and psf-Matched co-adds in a single task at the moment. But it doesn't NEED to right now, and running the command-line twice would have the same effect with the same performance. I added the capability for it to make both with the expectation that direct coadds are going to use psf-matched temp exps for their masking per DM-8290, and we need to start thinking of assembleCoadd working with both types of tempExps simultaneously. coaddBase.getTempExpDatasetName() is everywhere, and previously assumed that only one type of tempExp was used per instance of assembleCoadd. On this ticket, the assembleCoadd tempExpType is designated by maintaining state in a self.warpType instance variable; I am not attached to this implementation, and we can postpone until epic DM-8290.

• obs_monocam had some deepCoadd products like deepCoaddId, but no deepCoadd_tempExp in its policy file. obs_monocam does not have sufficient data products to make a monocam coadd, and it is out of scope of this ticket to make monocam coadds possible.

Jenkins is running now.

Paul, I added a commit that implements 1/3rd of your RFC-283. Changed the names of the data products ONLY, but not the filepaths, task names, and all the variables in the code. I updated some of the documentation. AssembleCoadd is in somewhat of a confusing state because some methods arguments with tempExp in the name, such as tempExpRefList; to a reader it would appear that they are called both Warps and CoaddTempExps. This added pipe_drivers and ci_hsc to the list.

There were a few data products in obs_subaru/SuprimeCam.paf and Hsc.paf that were unfamiliar to me, and only used in non-lsst scripts. What are these?

    deepCoadd_diffsrc: {

        template:    "deepCoadd-diff/%(filter)s/%(tract)d/diffsrc-%(filter)s-%(tract)d-%(patch)s.fits"

        python:      "lsst.afw.table.SourceCatalog"

        persistable: "SourceCatalog"

        storage:     "FitsCatalogStorage"

	tables:      "raw_skytile"

    }

    deepCoadd_directWarp_diffsrc: {

        template:    "deepCoadd-diff/%(filter)s/%(tract)d/%(patch)s/diffsrc-%(filter)s-%(tract)d-%(patch)s-%(visit)d.fits"

        python:      "lsst.afw.table.SourceCatalog"

        persistable: "SourceCatalog"

        storage:     "FitsCatalogStorage"

	tables:      "raw_skytile"

    }

    coaddTempExp: {

        template:    "coaddTemp/%(filter)s/%(tract)d/TEMP-%(visit)07d-%(ccd)03d-%(patch)s.fits"

        python:      "lsst.afw.image.ExposureF"

        persistable: "ExposureF"

        storage:     "FitsStorage"

        level:       "Skytile"

        tables:      raw

    }

the data product "coaddTempExp" appears in lsst/analysis – utils.py for example. Is this out of date?

Will you please review the final name change commit separately from the previous two.

The major change in this ticket has not thing to do with data product names but rather that makeCoaddTempExp and assembleCoadd makes 2 warpTypes simultaneously now. Please please please read my note (second bullet point in my first comment) above about assembleCoadd. There were two options: (1) users run "assembleCoadd.py ... config.warpType='psfMatched'" or (2) "assmebleCoadd.py config.makeDirect=False config.makePsfMatched=True" where option #2 mirrors the behavior of makeCoaddTempExp. I picked #2 for the reasons stated above.

• Please don't use the abbreviation "CTE" in the git log summary line. I'm sure it immediately clicks for you, but "CTE" in my mind is always tied to "Charge Transfer Efficiency".
• Docstrings need to be added, updated.
• Perhaps put some more thought into the revised APIs. Maybe this isn't necessary given that I'm going to be coming behind you to refactor a bunch of things.
• Don't dig through layers of encapsulation.
• Please consider using an enumerated type instead of names for the warp types.
• Don't set self.warpType.
• I've suggested squashing commits; maybe that's not necessary.
• Are there tests for the various cases (direct, psfMatched, direct+psfMatched)?

Thank you!

Do you have an opinionnon the new behavior of assembleCoadd (see notes in my 2 comments) which makes 2 coadds now?
warpType will be a argument to every single method in assembleCoadd (which also means that every method interacts with the butler. I know we can't assume they fit in memory). What this tells me is that ultimately we need a separate task that makes a coadd for a single warpType and a task that does All the Coaddition (direct, psf-matched, likelihood).

I don't mind having both warp types implemented in a single pass, especially since the behaviour is configurable. I assume you RFCed it.

But I'm starting to get the feel that this is all hacking around the current structure of the warping and coaddition tasks, and that what we should be doing is redesigning those. Maybe that's what DM-5766 is all about, and we can leave it until then.

Yes, I share that feeling.

For the record, and just to make sure Yusra AlSayyad doesn't catch any flak —

We discussed review procedures at the 2017-04-06 standup, partly in reference to this ticket, and partly because I've given misleading advice in the past.

Here, the word of the Developer Guide is law. In particular, while a review is still iterating, the ticket stays in status "in review". The reviewer should only mark a ticket as "reviewed" when they are satisfied that all their comments have been addressed or will be addressed: it's effectively permission to merge at the developer's discretion.

Note that we do not give any real weight to the classification assigned to reviews on GitHub PRs. In particular, if a ticket is marked in JIRA as "reviewed" but on GitHub as "request changes", them the JIRA status is the only one that matters.

That's relevant in this case, because this ticket is marked as "reviewed" but the PR on pipe_tasks is still marked as "request changes". Formally, I reckon that means Yusra can merge this whenever she's ready without further sign-off.

OK, this is finally ready to go. Paul Price, I made all changes requested except:

• Your suggestion to squash the two commits for (1) adding PSF-Matched data products and (2) changing the name of coadd temp exps to warps per RFC-283. From my perspective, it's already confusing that they are on the same ticket. Putting them on separate commits was a way to isolate them in case we need to revert one and not the other.
• MakeCoaddTempExp: whereas I agree with the idea, I didn't move the contents of the try block inside createTempExp to its own method now. There are 5 variables that are updated with every calexp (coaddTempExps, totGoodPix,numGoodPix, inputRecorder, didSetMetadata). With the current design, the new method would take 10 arguments. We could refactor this into processVisit() which then calls processCalexp() which updates a container of holding the coaddTempExps, totGoodPix,numGoodPix, inputRecorder, didSetMetadata. Outside the scope of this ticket but doable on DM-9610.

Major changes since first iteration:

• AssembleCoadd only makes one warp type coadd at a time
• PSF-Matched coadds are generated and tested in testCoadds.py
• WarpType is now an Enum
• createTempExp returns a Struct of a dict called "exposures" whose keys are one or both of WarpType:DIRECT and WarpType:PSF_MATCHED

Given that there's significant new stuff here since the original review, at this point I think I'd prefer if someone looked over it quickly.

Given that there's significant new stuff here since the original review, at this point I think I'd prefer if someone looked over it quickly.

Given that Paul Price is already familiar with the work, I guess it makes sense for him to do it? I believe he'll be back from wombatting mid-next-week — will that be soon enough for you?

I'm going to merge this after three Jenkins configs pass – to double checking nothing serious has changed since April.