DM-13293: Make BF kernel measurement code fully stack compliant

[mfisherlevine]

No description provided.

[r-owen]

This is a start on my review. I made it about halfway through the code.

[r-owen]

Please add proper task documentation.

I also suggest removing this reference as it does not appear to contain any useful information (unless I missed something).

[mfisherlevine]

Rewritten. But given that it's quite long and from scratch I imagine there will be comments on it.

[r-owen]

I feel this name is too vague in that it could easily be a task that applies b-f correction (which it is not). Consider something along the lines of {{MeasureBrighterFatterTask}} or {{MakeBrighterFatterKernelTask}}.

[r-owen]

Please rename all of these as per my suggestions for the task name above.

[r-owen]

I know you spent some time on this, so I hate to mention it, but please consider changing this to the usual format for multiple arguments, e.g.:

add_argument("--visitPairs", nargs="*",
    help="Visit pairs to use. Each pair must be of the form INT,INT e.g. 123,456")

This has several advantages:

• It is the standard convention for arguments that take multiple values.
• There is no need for the user to quote the data; just leave one or more spaces between each pair, e.g.
  --visitPairs 123.456 789,987 654,321
• It is easier to provide many pairs, as one can break the list between any pair using \
• It is easier to parse the result from argparse, which will be a list of strings, e.g. ["123,456", 789,987", "654,321"]

Here is the resulting help, which shows why I suggested spelling INT in all caps:

  -h, --help            show this help message and exit
  --visitPairs [VISITPAIRS [VISITPAIRS ...]]
                        Visit pairs to use. Each pair must be of the form
                        INT,INT, e.g. 123,456

[mfisherlevine]

Oh. My. God. THIS! 🤯 This is the answer I was looking for all along! How did nobody else mention that I was reinventing the wheel rather than shouting at me about my lack of regex knowledge! 😄

Thank you, this is indeed the elegant (and pre-existing solution) I needed. nargs="*" for the win! 🎉

It looks to me like I still need a (now very slimmed down) taskRunner to pass through the argument though, is that right? It now just looks like

    @staticmethod
    def getTargetList(parsedCmd, **kwargs):
        """Parse the visit list and pass through explicitly."""
        visitPairs = [(int(visit.split(",")[0]), int(visit.split(",")[1])) for visit in parsedCmd.visitPairs]
        return pipeBase.TaskRunner.getTargetList(parsedCmd, visitPairs=visitPairs, **kwargs)

[mfisherlevine]

I tried passing them through un-parsed with **kwargs and doing the parsing later so that the whole TaskRunner could be done away with but couldn't get it working. Could you confirm that you think this is the best that can be done please?

[r-owen]

I think having a custom task runner is very reasonable because of the need to pass extra arguments to runDataRef; that is a classic use case for a custom task runner. Fortunately it can be simple, as you say.

As to where to parse the visit pairs (converting them from a string "123,456" to a pair of integers (123, 456)): clearly it should be done before calling the task, which I think means it has to be done as part of parsing the arguments or in the task runner. Doing it while parsing the command is best, as that catches any problems as soon as possible and allows you to print the usual "I could not parse this command, here is the command help" message. The argument parser does this for other arguments using a callback specified by the action argument to add_argument. See class IdValueAction in argumentParser.py for an example of a callback, and how it is used. There are a number of similar classes defined nearby.

[mfisherlevine]

OK, glad I'm not doing it too wrong then. It seems like this one liner is very simple, and much more understandable (to me at least) than using callbacks etc, so as long as you don't think this needs moving elsewhere I'll just leave it as-is.

[r-owen]

I suggest refactoring run as per RFC-352: split run method into two pieces: runDataRef which does all I/O including unpacking the data and saving the result, and another method which does most of the work but does not do any I/O.

Typically that other method would be named run but in this case I suspect that's not reasonable, since you surely don't want to load all the visits into memory at once! I suggest you should have runDataRef call a method that processes one pair of visits. That method should be called something more descriptive and less misleading than run because it is not a method that "does all the work except I/O".

This will require one trivial tweak to the task runner to call runDataRef instead of run, but since this task has its own task runner that should be no problem.

Note that @czwa is doing this on DM-2639 for most of our existing tasks, in preparation for the move to SuperTask. So now is a good time to do it.

[yalsayyad]

Can confirm. This method should be renamed to runDataRef if you want to this to work as a CmdlineTask after DM-2639 gets merged. (Remember that all "dataRef" means in runDataRef is Gen2 butler objects vs Gen3 butler objects.)

[mfisherlevine]

For now, as discuss OOB, the method has just been renamed runDataRef() and all is well.

[r-owen]

The runtime error message is misleading because in this case the gains were not calculated. Also I think this log message and the exception message should match. I suggest you set a variable to the message and then use it for both the log message and the exception. That violates my suggestion above to defer formatting of log messages, but in this case we have to format this message anyway and fatal log messages are always shown.

[mfisherlevine]

I actually don't know why I was logging a FATAL as well as raising the RuntimeError tbh. I moved the message from the log into the RuntimeError so that it's accurate, and removed the logger message. I think that should cover this.

[r-owen]

I agree. No point logging and then raising.

[r-owen]

Please delete this debug message. It is a duplicate of the message on the next line and the one below is better because it provides the information regardless of whether the gains were computed or retrieved.

[r-owen]

"Preparring" -> "Preparing"

[r-owen]

I suggest hyphenating "cross-correlation" here and everywhere in the code.

[r-owen]

I suggest "DETECTOR" unless this code is specific to CCDs. Detector is the standard LSST term. At least I hope it is. @TallJimbo may have other opinions since he (or somebody) is using "sensor" in the butler gen3 schema. But we have lsst.afw.cameraGeom.Detector so I'm crossing my fingers.

[mfisherlevine]

Haha, I hope this too! It was me commenting that on RFC-484, and who made those changes to obs_lsstCam, so this is quite amusing that I went wrong here 😛

So yes, totally agree, and only 68 instance to change in this file 🤪

[mfisherlevine]

This might, however, lead to some weirdness/slight inconsistency, as the dataIds will still contain CCD as most obs_packages haven't been changed yet. I'll change it where I can, and we'll see where we end up is probably best. I'll let you know when it's time to rereview.

[mfisherlevine]

OK, this has now been done. I think all the places that ccd remains are more or less appropriate. It's slightly ironic that the convention we've chose to allow for dynamic keying of this is ccdKey rather than detectorKey, but changing this here and not elsewhere is both asking for trouble and well beyond the scope I think. If there are any instances you still object to do shout though.

[r-owen]

This is cryptic and duplicates the comment on the next line. and, to some extent, the comment at the top of the loop
I suggest just one comment that is clearer, or one here and one at the top of the loop. I think you are trying to say something like this (though I may have it wrong and you certainly should reword it as you see fit):

# Compute cross-correlation and means at the appropriate config.level:
# - "DETECTOR": compare the two visits
# - "AMP": compare each amplifier of one visit to ... (what? each amplifier of the other visit? the same amplifier of the other visit?)

[mfisherlevine]

Yes, this was just a garbage comment, thanks. It now reads:

            # Compute the cross-correlation and means at the appropriate config.level:
            # - "DETECTOR": one key, so compare the two visits to each other
            # - "AMP": n_amp keys, comparing each amplifier of one visit to the same amplifier in other visit```

[r-owen]

A few more comments. More to come.

[r-owen]

This task should be in bin.src not bin

Also I feel the name is too vague, as per my comments about the task name. I hope a more descriptive name for the task will lend itself to a similar name for the bin.src script.

[r-owen]

Please rename this file to (at least approximately) match the task name.

[r-owen]

Consider "the following import is required for 3d projection"

[r-owen]

The "or load a post-ISR image" is a bit confusing. Consider expanding this to "or load a post-ISR image (depending on the camera and configuration)".

I'm worried that the rest of this string duplicates too much information available elsewhere, and thus will be difficult to keep correct over time. Also the level of detail may fool readers into thinking that is all the ISR task does. Consider dropping it or dramatically shortening it (perhaps with a reference elsewhere).

[mfisherlevine]

Oh, you're quite right, I think this was ~copied from the main task and I forgot to change it, and indeed the bit about "loading an image" is just a lie. Totally rewritten.

[r-owen]

Please say more about the "successive over-relaxation method" and what it is used for in this code somewhere. Probably not here, but how about in the doc string for the task? I used a global search to find the _SOR method but it does not provide this information, and since it's a private method, its doc string is probably not the best place for it.

[r-owen]

Please expand "SOR" here and in other config field names and the method name _SOR. Our naming conventions require spelling things out unless the acronym is really common. I don't think SOR qualifies.

Failing that, our naming conventions require this to be renamed maxIterSor, which I hope is unpleasant enough to encourage a longer name.

[mfisherlevine]

Yes, agreed on all fronts. Have changed all instance of SOR to SuccessiveOverRelaxation, and changed the private method from _SOR() to public successiveOverRelax() and put the description in its docstring. If you feel this should be moved elsewhere in the codebase let me know where and I can put it there, though I feel that there is likely to be pushback if it goes in one of the appropriate places as it's a numerical method with potentially quite a few iterations, implemented in python (and probably not even very efficiently at that), so would prefer to keep this hidden away in here for now if possible.

[r-owen]

Please say more about what measurement this is the sigma of.

[mfisherlevine]

I've changed the _generateKernel() to be public (for a few reasons) and have said to see its docstring for more info. I hope that's sufficient.

[r-owen]

Please document the units.

My apologies if this is obvious. I don't know what is being cross-correlated to what (other than presumably it's some kind of correlation between the two visits in each pair). That's the sort of thing I'd like to see documented in the task doc string.

[mfisherlevine]

Yes, sorry, it was obvious to me, but perhaps not those unfamiliar. It's correlating two images in detector-space, so the answer is just pixels. Have added, and will tweak the docstring too.

[r-owen]

Could "/" sensibly be replaced with "and" or a space? Again, my ignorance showing.

[mfisherlevine]

No no, quite right, "and" makes more sense.

[r-owen]

Please remove the hyphen between "correction" and "factor". Also I suggest you remove "scary" (sorry to be a spoilsport).

If possible please explain more about this parameter. For instance:

• How is it used?
• How would a user determine if the value is too large or too small?
• What sort of values are "reasonable". For example if I want to make it a bit bigger or a bit smaller, I'd like some idea of what "a bit larger" or "a bit smaller" would look like. Also if there is an intrinsic limit on sane values, that could be helpful.

If any of this is too complicated then it should, perhaps, be put elsewhere, e.g. in the main task documentation, with a pointer here.

[mfisherlevine]

I've done the first bits (the phrasing etc), but the explanation is a bit tricky. Essentially this is something that probably shouldn't be touched except by experts, but I made it a config option because having a hard-coded number in the code filled me with horror, especially as I can imagine this being different in some cases. How it's calculated should indeed be added, and probably methods for doing so added to this module, and therefore those referenced here, but given that hasn't been done yet, I'm going to have to punt on this for now.

[mfisherlevine]

I think what I'll do is ticket adding the code to do so to this module, and add the ticket number to this docstring, saying that it should be updated once DM-XXXXX is done. Does that sound like a reasonable solution?

[r-owen]

Yes

[r-owen]

It worries me that a method whose name starts with validate is not run by the validate method. I can see two solutions. The simplest is to rename this method and/or move it to the task (and, presumably, call it from the task constructor, which is where I assume it is being called now). Another solution is to split it into two parts: a non-logging part and a logging part:

• The non-logging part (which acts like validate) should either keep the current name and be called from validate or else be renamed to validate, as you see fit.
• The logging part should get a name that does not start with validate (since it logs but does not validate). I assume the task constructor will call that. I have no strong opinion about whether the non-logging part lives in the config or the task; I can see arguments both ways.

I wish there was a better way to figure out if the post-ISR data has been appropriately processed -- one that did not rely on internal knowledge of the possible ISR tasks. But I don't know of anything yet. Gen3 butler should support this better and meanwhile this seems like a sensible and pragmatic approach.

[mfisherlevine]

OK, glad this wasn't too offensive. I think that discussing with someone before they thought that this belonged in the config class rather than the task, but I can see it either way. I've renamed the method to checkIsrConfig() to avoid the v-word, and moved it to the main task. My reading of what you wrote is that this is sufficient, but let me know if you want any further changes there.

[r-owen]

If there are parts that can usefully validate the config without logging then they should probably be moved to validate (or a method called by validate in the config. But otherwise your solution seems best.

[mfisherlevine]

Yeah, I'm torn. It's not really validation is the thing; they're not intrinsically invalid, they're just ensuring that people aren't Doing It Wrong™️, but they're not testing for a non-compliant pexConfig, so I think it should probably stay as-is really.

[r-owen]

I'd say if it raises an exception then it's validation. We do this sort of thing in other places -- check that something was processed with required config settings before being allowed to proceed -- and we call it validation there.

However, I am not saying you have to split this into xConfig.validate (with no logging) and the rest of it. If it feels like a coherent bit of code to you then it's probably best to leave as a single method.

[mfisherlevine]

OK, I've just renamed the method to validateIsrConfig() config then, and left it as it was (after the changes I made before of course, so it still lives in the main class).

[r-owen]

Partial re-review. The new documentation is a big i mprovement. I'm not sure what the issue is with the conditional debug code.

[r-owen]

"appropriacy" is an great word (one new to me), but the definitions I found don't work here. Perhaps it would be more helpful to say that certain ISR steps must be performed. If they can be briefly explained then do it here. Then point to the config fields that specify the checks.

[mfisherlevine]

OK, will fix. But for reference, the Google definition I get is:

appropriacy
əˈprəʊprɪəsi/
noun
noun: appropriacy; plural noun: appropriacies

    the extent to which something is suitable or proper in the circumstances.
    "the appropriacy of the methodology employed"

which is the way in which I intended it, and seems to fit well, to my ears at least 🙂

[mfisherlevine]

I'm not sure what you mean by "the config fields that specify the checks" though, as there are none - the isr config is always checked. I guess you mean "the config fields that are specified by the checks", i.e. which fields are checked, right? If so, I'll mention the most important and most-likely-misconfigured ones, as the list is quite long, and the code can define the rest (they live in three very self-explanatory lists after all).

[r-owen]

By "the config fields that specify the checks" I meant the three task config fields that specify which ISR config fields are to be checked. I would avoid listing any specific ISR config fields here as information could easily go out of date. (That said, one or two really obvious ones is fine if you think an example will help -- but even then I'd be more inclined to provide that information in the documentation of the 3 config fields that actually specify which fields to check).

The first definition I found of appropriacy was specific to linguistics, but I looked again and agree it can also be used as a synonym for "appropriateness". Nonetheless, I think a better word here would be "compatibility", "validity", "correctness" or something like that. To me "appropriateness" implies style checking, not validity checking.

[mfisherlevine]

Ah, I see, the confusion (as you point out below) was you thought that these hard-coded lists were actually config options, as they should be. I've changed the phrasing already, and will make these config options now.

I'm a little confused about what you mean by "I would avoid listing any specific ISR config fields here" though - don't I have to list them in order to use them? Are you suggesting that the defaults should be empty, bar the really obvious and essential ones, and then allow users to configure in each obs_package? This is somewhat against what I was trying to do, in protecting people from accidentally applying "corrections" which are on by default unless overridden in the obs_package, which would invalidate the results. Maybe we can have a quick chat on Slack about what you'd like to see here. If so, let me know when's good (typing is fine).

[r-owen]

All I meant was I would avoid listing them in documentation. Being a bit vague in the documentation reduces the danger that the docs will get out of synch with the code. That said, some examples (preferably in the main task's doc string and/or the help for the fields in question) is helpful. For instance if you know that the ISR task must assemble images you can say that, rather than list a specific flag that makes it happen.

[r-owen]

Please spell out "PTC"

[r-owen]

I suggest removing "the" before "config.level", but overall this is excellent -- very clear and helpful

[r-owen]

I suggest removing "in pixels" here, since it's documented in the config

[mfisherlevine]

I dunno... I think that it reads as it should, and might save people searching around if they're confused. I think it adds clarity, sounds right, and as it will never change having it defined in multiple places is no problem. Unless you feel strongly that it should go I'd rather leave that there. However, as this is defining the shape of the output of the whole task I've moved it up in this section, and I think the whole thing reads a lot better now. Fro convenience, this section now reads:

    - Once the cross-correlations have been calculated for each visit pair,
      these are used to generate the correction kernel.
      The maximum lag used, in pixels, and hence the size of the half-size
      of the kernel generated, is given by config.maxLag,
      i.e. a value of 10 will result in a kernel of size 2n-1 = 19x19 pixels.
      Outlier values in these cross-correlations are rejected by using a
      pixel-wise sigma-clipped thresholding to each cross-correlation in
      the visitPair-length stack of cross-correlations.
      The number of sigma clipped to is set by config.nSigmaClipKernelGen.

Let me know if that's OK, or if you want changes (or the mention of "in pixels" removed).

[r-owen]

I think it's best to use your judgement here.

[r-owen]

This new task documentation is great!

[mfisherlevine]

😊

[r-owen]

I'm not seeing the issue; can you please explain more (here in github is fine, no need to modify the code)

[mfisherlevine]

The solution here is going to be pull out all the commented code and ticket enhancing the debug plot functionality I think. But to explain though:

the issue is that to generate the correct titles for the debug plots you need the objects, but you don't want to be passing everything around just to be able to query things that aren't really used. However, a massive block of code to generate the titles doesn't really belong in the main routine, and even less so as it mostly won't even fire.

I don't think there's a nice solution to this, and we'll probably see this code reappear in this place, though hopefully a little more cleanly/concisely, if/when that ticket gets done, but for now I think just removing it is fine.

[r-owen]

Please consider rewriting in active voice (the usual Python standard for doc strings) e.g. "For each amp apply borders (what does that mean?) rescale by gain, and subtract sigma-clipped mean"...

[r-owen]

Please spell out PTC

[r-owen]

Any chance of figuring this out before merging? If not, please file a ticket and reference it here.

[mfisherlevine]

I have made it log warnings the background is found to be above a configurable level, so people in the future will know if it's subtracting something large. I think this should do it.

[r-owen]

I have to run, but here are some comments. I found a few new things, but nothing big. It's looking very nice.

[r-owen]

Please remove this and all other from __future__ import... and from builtins import... statements from all python files (unless any are needed by Python 3, which seems unlikely).

[r-owen]

Please add __all__ = [...] to this and all other python library files (before the first import statement)

[mfisherlevine]

👍 to the __all__, but other modules put this after the imports, and indeed I get an E402 error for every module level import that follows this, as they have to be first according to that rule. Is there a reason you ask for this to be first?

[r-owen]

PEP8 changed at some point. It used to require that some imports went before __all__ and then changed (I think a few years ago). Now __all__ should go before imports but after the module level doc string (if there is one).

Here is the current text:
https://www.python.org/dev/peps/pep-0008/#id24 or look up PEP8 "Module Level Dunder Names". I am shocked that your linter is complaining. Mine accepts __all__ in the right location and has for years. Perhaps you put it above the module level doc string?

[r-owen]

Please add a question mark.

Also please consider removing "using the PTC method" or putting it in parenthesis. The important point is "should this code measure amplifier gains?" How they are measured is arguably an internal detail (though one that may be important enough to document).

Please spell out PTC if you retain it in the help string.

[r-owen]

Nice! However, the backslashes are unnecessary, since you are using single quotes inside a double-quoted string. Also there is an extra single quote after ccd.

[r-owen]

Sigma of what? Please say.

[r-owen]

Thanks for including the keyword

[r-owen]

Sorry I didn't catch this earlier, but all our existing command line options are lowercase with hyphens between words so please do that for the visit pairs argument. Use the dest keyword to specify the name for the parsed data, e.g.: add_argument("--visit-pairs", dest="visitPairs", nargs="*", ...). This change will require a one or two tweaks to existing task documentation.

[r-owen]

I suggest making these three fields config parameters, so an obs package can plug in the appropriate variant ISR task and tweak the tests accordingly. (I thought they already were config fields, which explains an earlier confusion -- I had suggested that a help string point to the config fields which specify these tests).

Unfortunately the field name should probably include "isr", e.g. "isrMandatorySteps", "isrForbiddenSteps" and "isrDesirableSteps" -- with a possible future "isrUndesirableSteps".

If you make this change then I also strongly suggest failing if a specified ISR config field doesn't exist (e.g. by testing and raising, or omitting the test if the resulting error message is clear enough). Otherwise you risk failing to test something important due to a spelling error. In fact that is another argument for making the tests more visible by making them part of the config.

[mfisherlevine]

OK, I've moved all this to configs as you said. For the checking, what I've done is to remove the protecting blocks that were in there for the forbidden/mandatory catagories so that you get an error if they're not found, I think that should suffice. This made sense too as they're now overridable per-obs_package, so no reason to be forgiving if they're not there.

If you could just take a look and check that the way I've gone about this is OK with you I think that would be good, thanks.

[r-owen]

I just looked at the code. The new config parameters look fine (though I'm disappointed that one still needs to be hard-coded -- assembleCcd.doTrim; if you used dotted names for subtasks you could avoid that). The code I found using these is the validateIsrConfig method and it still logs a warning if a config parameter in one of these lists is missing -- rather than failing. So I'm not sure why you said "you get an error if they are not found". I'd prefer an error, but I think what you have will do.

[mfisherlevine]

I think this is the last point remaining, but I've been struggling to understand here. What do you mean by "if you used dotted names for subtasks"? I tried to work it out but failed I'm afraid. Do you think you could give me a tiny example? I think regardless of whether it really needs changing I would learn something here.

Regarding the behaviour; mandatory/forbidden now raise, but desirable WARNs if wrong polarity, and logs an INFOs if the key is not found. I like this behaviour because it suggests to the user that they should configure the equivalent keys in the obs_package config, but this could easily be changed to a raise if not found if you think that's better (though I think it has to WARN if wrong, otherwise it's not "desirable", it's mandatory.) Does that sound good?

I'm totally happy to change this until you're happy though, so if you can confirm what you'd like to see instead I'll go ahead and make the changes (especially if I can move the hard-coded isr.assembleCcd.doTrim into the mandatory config).

[r-owen]

I just mean that the name in the config could be "assembleCcd.doTrim". Parsing that is a bit more work, as getattr and setattr cannot handle multiple levels in one call. However, a simple loop like the following would suffice (this is only lightly tested):

def getDottedAttr(obj, name):
    """Get an attribute that may be several levels deep, e.g. foo.bar.baz
    """
    if not name:
        raise ValueError("Name is blank")
    try:
        nameList = name.split(".")
        value = getattr(obj, nameList[0])
        for subName in nameList[1:]:
            value = getattr(value, subName)
        return value
    except Exception as e:
        raise AttributeError("Could not find {} in {}: {}".format(name, obj, e))

[r-owen]

And just to make it clear: I suggest using this function for all values you are trying to get, not just ones you know are dotted. The loop is not run at all if nameList has only one element, so it'll be quite efficient.

[r-owen]

Consider making this debug level. I worry about users complaining about excessive verbosity. In which case the "beginning" log message may want to be "Compute gain estimation for detector %s"

[r-owen]

Please don't leave in commented-out code (at least not without a ticket #). It is an interesting issue. It seems like a great idea to include the info, but it's weird to mix data with metadata.

I think either of these would be a cleaner solution but I don't know if it's worth the work:

• Make kernels an object that has an explicit field for level.
• If you might want to generate kernels at amp level and also kernels at detector level for a given camera, then make a separate dataset type for each.

[mfisherlevine]

I think both of those are good suggestions, and I especially like the latter. However, I will just remove for now, and if/when we decide we want that I can make a ticket and new datasets etc, as we're not really losing any code here.

[r-owen]

A few minor corrections. The main ones I'm concerned about:

• Please delete all commented-out and otherwise unused code or write tickets for it and reference them in the comments.
• I worry that there are too many "info" level log messages. I encourage you to take another look to see if any should be lowered to "debug" level.
• There are pep8 violations (including at least one bare "except", one line that is too long, and lots of missing spaces around "+" and "-".
• When you are ready to merge this I suggest you squash all the commits together.

[r-owen]

I think the description of the DETECTOR case is wrong: the code never modifies exp and always returns either an exposure or a collection of amplifier exposures.

I am relieved, as I think this code should either always operate "in place" or treat the input exposure as read only, rather than do one for DETECTOR and the other for AMP.

[mfisherlevine]

100% agree. I don't know why it said this. I think it's because these docs were adapted from the original code a little too blindly, this was never the case, and certainly code should never work like that!

[r-owen]

A helpful note, but I suggest pexConfig -> config (and look for this elsewhere)

[r-owen]

I suggest you delete this comment. An alternative is to file a ticket, include the ticket number in the comment and consider moving the comment closer to where one of these variables shows up. Still...making a copy does seem reasonable. I'll be impressed if you find you can do without it.

[r-owen]

I suggest you use the term "exposure", "image" or "maskedImage" as appropriate -- both in method names and in documentation. In this case it would be "exposure". Thus _prepareExposure. Also, the method name sounds like it operates in place, but I'm not sure what else to suggest. Perhaps _makeCroppedExposures (even though it should be singular if level is DETECTOR)?

[mfisherlevine]

Yeah, I agree, and I'm not too worried about the singular return length with that method name. I've gone for _makeCroppedExposures().

[r-owen]

Another pexConfig -> config

[r-owen]

Please provide more documentation so the user has a clear idea what output to expect for a given input.

[mfisherlevine]

Have removed all this code as it was actually unused (it only features in making titles for debug plots), and have just dumped it in DM-15305 for now.

[r-owen]

Please document the default value (which is config.xcorrCheckRejectLevel)

[r-owen]

Still not documented?

[mfisherlevine]

Not sure, but I think you just need to scroll down. I see:
rejectLevel : `float`, optional This is essentially is a sanity check parameter. If this condition is violated there is something unexpected going on in the image, and it is discarded from the stack before the clipped-mean is calculated. If not provided, a default value of 0.2 is picked up from the task config.

[r-owen]

I suggest: If not provided then config.xcorrCheckRejectLevel is used. It is risky to document the default value of a config parameter anywhere (except in the config itself, where it is set) because it may change, and of course obs packages may override it.

[r-owen]

You don't need the float in Python 3. Also please add spaces around the +

What should happen if mean1 == mean2 == 0?

[mfisherlevine]

I think that if the means are identically zero then things are sufficiently bad that a division by zero is OK actually.

[r-owen]

I suggest deleting this assignment as it is not being used

[r-owen]

I don't see any use of this in the package. Can you get rid of it, along with _crossCorrelateSimulate above? If you have unresolved issues to work out then at least file a ticket and add a TODO message to the doc string for both of these functions and consider moving it to a separate module.

[mfisherlevine]

OK, this has been tidied up significantly. The code must live on, and I think this is the right place for it, but it certainly needs a little more work, as there are some known problems. I've written a new function that unifies the generating images part and allows switching which code is called to do the calculations, and filed DM-15756 to finish up that bit of work, which will then remove the vestigial function here I think. If you have any comments on how you think that should go (including if you disagree that it should be in this module) do please leave them on the ticket, but I hope this is OK for now.

[r-owen]

That sounds fine to me.

[r-owen]

You don't need the float in Python 3

[r-owen]

I found a few pep8 violations; please run flake8 before merging and also look for badly formatted lines.

[r-owen]

Please don't use bare "except" and please run flake8

[r-owen]

This line is too long and the formatting is confusing and not compliant with our standards (e.g. spaces around "+"). I suggest:

                    dim_xy = diffAmpIm[border + xlag: border + xlag + w,
                                       border + ylag: border + ylag + h,
                                       afwImage.LOCAL].clone()

[r-owen]

Please add spaces after ":" and around "-" (or before, in the case of negating border). I suggest a global regex search for + and - without surrounding spaces

[mfisherlevine]

Done! (I think)

[r-owen]

I don't see the changes I requested. I also found one more worth considering: using scipy.signal.convolve2d instead of python loops.

[r-owen]

Please add better error handling, as per the comment above.

[r-owen]

Is func.shape guaranteed to be square? If not, I would expect the index to be different for i and j (e.g. [0] in one case and [1] in the other (e.g. as above for "Calculate the initial error"). Even if it is guaranteed to be square now, I suggest changing this to be less surprising to users and in case rectangular support is added in the future.

[mfisherlevine]

Fair point. I think I will add an assert though as well, because a non-square kernel sounds like madness to me, in this case at the very least.

[mfisherlevine]

Now done.

[r-owen]

Both here and elsewhere in this method you would probably get better speed and possibly code clarity by using scipy.signal.convolve to apply one kernel to func and one to source: resid = func convolved with [0, 1, 0] [1, 4, 1] [0, 1, 0]] - source convolved with [[0, 0, 0] [0, 0, 0] [dx**2, 0, 0]]. It's a 1-liner (modulo defining the two kernels, which are always the same and can be created once and re-used).

[mfisherlevine]

As discussed OOB I'm going to leave this as-is for now because despite being both ugly and slow, it will only save milliseconds of compute to change it, and testing that the behaviour is identical will take a lot longer.