Colin do you have time for a review (ip_diffim, pipe_tasks)?

Summary of changes:
DipoleMeasurementTask was just a default configuration + some documentation. There were two options for cleaning this up: make a very light DipoleMeasurementTask which inherited from pipe.base.Task consisting of a config and some documentation, or just delete it (and move the config + docs to ImageDifferenceTask. I chose deleting it, because I envision the processImageDifferenceTask as the reusable task to house the default configs for running SingleFrameMeasurement on image differences and the documentation on what those default plugins are and do. I see this ticket as an intermediate step and moving the config into the current ImageDifferenceTask as a step in the direction we wanted to go. I deleted the docs rather than moving them, because ImageDifferenceTask does not yet have a modern pipe task documentation.

Note: This changes the field name of the dipole classification from "dipole_classification" to "ip_diffim_ClassificationDipole_value" (and "ip_diffim_ClassificationDipole_flag") analogous to "base_ClassificationExtendedness_value"...

I noticed one algorithmic change: the measure() function will call fail() without setting an output value if it does not have both a positive and negative flux. I’m not 100% clear on what the semantics are meant to be when a classification plugin "fails", but my inclination is that in this case it should set the output value to 0. I have less of an opinion on whether this also requires a fail flag (the measurement that it is not a dipole may still be reliable). This is overall perhaps a peculiar case where we are only interested positively identifying dipoles, and there is little practical difference between “we couldn’t tell if this is a dipole” and “we know this is not a dipole”.

It is unfortunate that we have to duplicate the dipole measurement configuration in tests and examples, but I don’t see an easy way around that.

Everything else seems like the the right thing to do. I concur on the docs decision.

Your desired behavior for the classification to be set to 0 by default was being achieved: Because fail() is being called on purpose rather than performed after an exception, the block of code that checks the dipole-conditions are still run and the value is set. In the case where _pos or _neg are nan: nan < anything will evaluate to False and classification=0. In the case where _pos or _neg flags are set, the conditions will be evaluated and classification will be set to 0 or 1. I added a couple comments to make this control flow more clear.

What is new here is the classification flag. You're right that by calling fail() to set the flag if the _pos/_neg fluxes are NaN, it is being hyperactively set. In response to your review, I changed the logic so that the flag is manually set only if the _pos or _neg flags are set, and not if the _pos/_neg are nan. Of course if there is an exception, it also gets set on its way out and the classification value will be NaN.)

Yes, having to duplicate the configuration is unfortunate. David Reiss, this probably affects you most immediately as you are editing and adding dipole plugins. Would you take a look before I merge to master?

David was also not excited about the code duplication. I added a helper function that makes a default config with the typical dipole measurement config as a new commit. Since the last commit is unreviewed, would @ctslater or one of the watchers take a peak at the last commit in both pipe_tasks and ip_diffim?

Can you please explain why you created the makeDipoleMeasurementConfig function instead of subclassing SingleFrameMeasurementConfig and defining the desired defaults using the setDefaults method? I see that your function takes a config as an argument, and I suspect that's key, but I didn't spot any code using that method. I suppose it's not a traditional use case anyway, since you are adding a plugin instead of creating a new task, so a function is probably as good as a new config class.

I'd like to close this. I put a new version on u/yusra/DM-3515 that does not remove the old DipoleMeasurementTask and does not touch the imageDifference.py driver in pipe_tasks.

This will all be replaced by David Reiss's new DipoleFitTask soon anyway. The only impact is that the name of the slot in which the current dipole classification has been changed from "classification_dipole" to "ip_diffim_ClassificationDipole_value"

I will merge tomorrow, unless I hear from any of you watchers.

I added a few minor comments to a new PR. I was not able to run the tests or examples today, as I have had to rebuild the stack this morning. One thing to check: the testDipoleFitter.py tests that I just merged compare the results to the dipoleMeasurementTask. Can you just verify that those tests still pass?