Here are the differences that I have found between HSC and LSST for reduceFrames vs. singleFrameDriver:

For charImage in LSST or calibrate in HSC which provides the initial catalog.

for the final catalog in the calibrate stage for LSST or measurement stage in HSC :

There are probably some things that I missed because of the many name and algorithmic changes that have happened.

For the stacking code:

We are getting very close to being ready to run the HSC RC dataset processing with the LSST stack (DM-6816), this ticket being one of the blockers. The final decision for the settings listed above should be locked in in the next few days. If anyone has any opinions, please note them here asap (here's looking at you Paul Price, Robert Lupton, Jim Bosch...anyone else?)

charImage:

Variance Mask | 'DETECTED', 'DETECTED_NEGATIVE' | 'DETECTED', 'DETECTED_NEGATIVE', 'BAD', 'SAT' | Keep LSST - Agree |

calibrate:

stacking:

Thanks Paul Price!

My only possible disagreement with Paul Price's recommendations is assembleCoadd.doWrite - I think we want the behavior to be for AssembleCoaddTask to write when it's running on its own, but not to write when run as part of CoaddDriverTask (since that writing is presumably done elsewhere). We may also need to worry about what its value should be when running SafeClipAssembleCoadd directly, but I don't know exactly how that logic works.

I'd rather specify the initial FWHM in arcsec, not pixels. And the kernel size can be calculated from the FWHM, so the default should be "use the FWHM". Both of these proposals assume that the plate scale is available, which must be true given the HSC defaults. This is a non-robust part of the current code – is there an issue to do a better job on the bootstrapping?

Otherwise, I defer to Paul/Bob/Jim

Paul Price: The noise seed multiplier allows for the random seed for noise replacement to be set:

doc = 'The seed multiplier value to use for random number generation.  0 will not set seed.'  

It gets set based on the exposureId here.

Jim Bosch: I believe the logic works the same for AssembleCoaddTask and SafeClipAssembleCoadd as the latter inherits the config from the former. In fact, you get the latter unless you explicitly specify --legacyCoadd when running assembleCoadd.py.

As for CoaddDriverTask, in order to turn off the persistence, I think that config (i.e. self.assembleCoadd.doWrite = False) would need to be added to the list here. Assuming Paul Price will be using the driver for the processing in DM-6816, should I add that on this ticket? I hesitate because I'm not sure what you mean by:

(since that writing is presumably done elsewhere)

Robert Lupton: I believe this change would need to be the subject of a separate issue (and, yes, HSC is making use of the plate scale).

We definitely want to set a seed for random number generation, but I'd be very surprised if we weren't setting the seed for HSC. I suspect the docs are wrong (or at least misleading), but this merits a closer look.

(since that writing is presumably done elsewhere)

The behavior I'm looking for is:

• When we run assembleCoadd.py on its own, it writes deepCoadd by default.
• When we run coaddDriver.py, it writes deepCoadd_calexp but not {{deepCoadd} by default.

Whatever we have to change in the configuration to make that happen is fine with me.

Ah, ok, I'll look into the logic to see what's happening (but perhaps Nate Lust knows off-hand?).

The noise seeding code in HSC is here and the default here is indeed 0.

A quick search for noiseSeed on GitHub doesn't reveal any overrides (except in the ticket2019.py test file in meas_algorithms), but I could be missing something.

Note that the noise seeding line you linked to above doesn't actually run if noiseSeed == 0, due to the if statement above it. Instead, it seems to fall through to here, which uses the afw.math.Random default constructor, which uses a seed value of 1. That's quite a bit more confusing than it ought to be, and it might be worth opening another ticket to fix that. Anyhow, it does seem like 1 is the right value to use, as that will ensure we get a different (but still deterministic) seed for each exposure we process.

Paul Price See the following commit:
https://github.com/lsst/obs_subaru/commit/fdd0416ea5b972775db1b70b8b9c94a46bd70362

Do you still agree with your proposal above about the Astrometry maximum pixel offset?

Note that with the override, the following are the current LSST config pars running HSC data:
config.charImage.astrometry.matcher.maxOffsetPix=300
config.calibrate.astrometry.matcher.maxOffsetPix=750

Hmm. I guess we should leave it as it is then. Thanks for looking into that.

I'm wondering about the decision to use 3.5 pixels on LSST vs. the 1" on HSC for the Width of initial PSF above. With a pixel scale of 0.168, these two values don't agree (1/0.168 = 5.95). I'm sure you all realized this when making the recommendation, but I just wanted to double-check.

A few more cases are:

We've recently noticed that these values are set too high on the HSC side. The LSST values are good.

This is the point of my comment "I'd rather specify the initial FWHM in arcsec, not pixels."

The default value should probably be around 1", but we need to be more robust about how we use it. I'm worried about using 0.6" for LSST (with expected median image quality around 0.8"). Is Paul Price's comment based on CR rejection at 3 pixels even in good seeing (i.e. there's no need to go to 1")? If so, I might reconsider my recommendation to go to arcseconds. Was that what Paul meant?

I've always considered that the initial PSF should be about the smallest seeing one expects to encounter, because it's used in the initial CR rejection. In that case, setting it to 3 pixels seems reasonable since the pixel size in a camera generally scales with the expected best seeing. I don't recall why this was changed from arcsec to pixels (it was part of the big processCcd overhaul), but I don't think it's an awful decision.

I've always figured pixels is a better unit than arcseconds because cameras tend to be built with pixel sizes that correspond to their expected seeing, and hence values in pixels would need to change less often across different instruments. Of course, there are enough cameras for which that's not the case that I'm not sure that's a useful argument.

There's also a technical issue that we've banned angle units other than radians from our code, and pex_config doesn't support smart Angles (DM-6885).

Summary of adopted changes:

charImage:

calibrate:

stacking:

** NOTE: this slightly changed the behavior of image characterization and detection causing testProcessCcd.py in pipe_tasks to fail with:

  File "tests/testProcessCcd.py", line 139, in testProcessCcd

    self.assertAlmostEqual(bgMean, 191.51202961532354, places=7)

AssertionError: 191.51595080958367 != 191.51202961532354 within 7 places

and then

  File "tests/testProcessCcd.py", line 140, in testProcessCcd

    self.assertAlmostEqual(bgStdDev, 0.28333327656694213, places=7)

AssertionError: 0.22492169148323429 != 0.2833332765669421 within 7 places

  File "tests/testProcessCcd.py", line 142, in testProcessCcd

    self.assertEqual(len(src), 185)

AssertionError: 178 != 185

  File "tests/testProcessCcd.py", line 143, in testProcessCcd

    self.assertEqual(numGoodPix, 1965540)

AssertionError: 1966762 != 1965540

  File "tests/testProcessCcd.py", line 145, in testProcessCcd

    self.assertAlmostEqual(imMean, 0.99578990765845121, places=7)

AssertionError: 0.99296421356520304 != 0.9957899076584512 within 7 places

  File "tests/testProcessCcd.py", line 146, in testProcessCcd

    self.assertAlmostEqual(imStdDev, 95.645409033639211, places=7)

AssertionError: 95.646024055615044 != 95.64540903363921 within 7 places

  File "tests/testProcessCcd.py", line 149, in testProcessCcd

    self.assertAlmostEqual(psfIxx, 2.8540775242108163, places=7)

AssertionError: 2.8540469922966296 != 2.8540775242108163 within 7 places

  File "tests/testProcessCcd.py", line 150, in testProcessCcd

    self.assertAlmostEqual(psfIyy, 2.17386182921239, places=7)

AssertionError: 2.173868758768284 != 2.17386182921239 within 7 places

  File "tests/testProcessCcd.py", line 151, in testProcessCcd

    self.assertAlmostEqual(psfIxy, 0.14400008637208778, places=7)

AssertionError: 0.14397371221988944 != 0.14400008637208778 within 7 places

The test values were updated accordingly.

Finally, this also changed the result in the lsst_dm_stack_demo, so
the corresponding expected files need to be updated. The main difference
is the number of output sources due to the activation of junk suppression
(which decreases from 5962 to 4624 in the dected-sources.txt files).

The following figure demonstrates this (having done a simple RA/DEC match
between the old and updated results files, with 4441 matches found):

and the following shows the difference in PSF flux for the matched sources

Can you plot the spatial structure of the outliers in the delta magnitude plot?

Voila...

You can see that the outliers lie mainly around the bright objects...junk suppression again. (And sorry about the axes...didn't want to spend too much time matching things up!)

That was what I hoped you'd see – thanks! If we mask the areas with lots of junk suppression how do the outliers look?

I'm not sure what you mean by "look" (spatial distribution, type of object, ...?) I can make the right plot above with much smaller dynamic range if you'd like.

Here:

That's a better plot than the one I suggested.

Interesting. Do we know what's happening at the left of this plot? Did the junk somehow pull the background estimation??

I believe so. And note that when we mark RFC-212 as implemented, I believe a ticket will be added to look more deeply and quantitatively into the effects of the junk suppression.

Paul Price, would you be so kind as to have a look at this to make sure I've correctly adopted the consensus defaults?

Jenkins passed through lsst_sims lsst_distrib ci_hsc, but fails the demo as it doesn't pull down a branch. I've pushed a branch to lsst_dm_stack_demo with the Linux64 updates (and see above for comparisons with the previous expected results). John Swinbank has kindly offered to produce the same for DarwinX86 results when he gets a moment this afternoon.

I'm surprised to see the SubtractBackgroundConfig.binSize being reduced, but we're not setting useApprox=True. I think we should turn on useApprox by default.

Yeah, that is currently being set as a config override in obs_subaru. There has been hesitation to default useApprox to True (see DM-2920), but I'm not sure what the current temperature is on that. John Swinbank, would adopting this require further formal approval, or can we take Paul Price's recommendation?

+1 on Paul Price's recommendation to turn on useApprox by default. I don't think it makes any sense to shrink the binSize unless we also do that, so if we feel we already have permission for the latter, I think that implies permission for the former (I don't recall whether there have already been RFCs or community posts on the changes in this ticket - we should probably have something if we haven't announced them already).

My reading of DM-2920 is that Lauren MacArthur reckoned useApprox didn't actually work correctly ("config setting of approxOrderX/binSize are not being assessed properly, nor is the behavior of the given undersampleStyle being executed"). If you're confident that it does work, I've no objection to enabling it.

I agree that an RFC summarizing the changes being made on this ticket would be appropriate.

Not quite. I do believe useApprox works properly. The issue was the ugliness of the code I had to write to make sure it would properly check for bad sampling settings. See here for details. It was only the hacked-up nature of the code that led to the hesitation in setting the default to True. Much processing of HSC data with useApprox=True confirms it is indeed working properly.

Ok, I think the solution is to adopt it (I agree with Jim Bosch & Paul Price that the two need to be adopted in tandem) and I will write an RFC for all of the changes on this ticket collectively.

Robert Lupton, quick update: having now set useApprox=True the apparent gradient is gone. Deep interpretation of any differences is difficult as the demo previously ran with no junk suppression and performing background estimation via interpolation.

Some comments on GitHub PRs.

I'm concerned about making applyUberCal=True the default because it will break cameras for which jointcal or meas_mosaic isn't working.

Paul Price: I've updated to keep applyUberCal=False as the codebase default, but am overriding it to True in obs_subaru by default for HSC coaddition. Also, because the QA analysis script makes use of the extendedness value in the forced coadd results, I've added the following to config/forcedPhotCoadd.py:

config.catalogCalculation.plugins.names=["base_ClassificationExtendedness"]

Are you ok with these changes?

Yes, thanks!

Thanks to everyone for all of the input on this one.

Merged to master.