ccdproc icon indicating copy to clipboard operation
ccdproc copied to clipboard

Consider revising the Combiner machinery into a class heirarchy

Open eteq opened this issue 7 years ago • 2 comments

I was trying to use Combiner recently, and I found it a bit awkward to use because a single combiner class has all the methods. So I want to through out an idea for a new way to lay out the combiner:

  • Have a CombinerBase or something like that which implements most of the bookeeping - pretty much everything except the *_combine methods. Include a combine_images abstract method.
  • Have separate MedianCombiner, SumCombiner, and AverageCombiner classes that basically just implement combine_images, using parameters from the init function, not method keywords.
  • Leave the existing Combiner, but it basically is just a compatibility class that wraps the above (might be deprecated in the future, but could also be left in with just a pointer that the class heirarchy is the "new" way).

My reasoning behind all of this is that it makes it much easier to use these classes in a pipeline, because in this scheme, you can create a *Combiner and then just call combine_images, and a downstream user can swap in various combiner types without modifying their pipeline. It also makes implementing more complex uncertainty propogation schemes a la #569 much easier

eteq avatar Oct 23 '17 17:10 eteq

@eteq it is an interesting thought. We do have the combine function to provide an interface that might be more useful for pipeline production or general use of it.

I do notice that it is not included in the combining images documentation, and should be added to the top of that.

Nonetheless, the Combine class really should be re-done with a c backend that could significantly optimize it especially for memory usage. At that time, it would make sense to step back and take a thorough look at it and what would be the best way to rebuild it for performance and usage now that we have a functional use of it.

crawfordsm avatar Oct 24 '17 09:10 crawfordsm

Gotcha @crawfordsm - yeah, re-optimizing is definitely a good time/excuse to do this. And I didn't know about the combine function so putting it in the docs is definitely a good idea.

To be clear, though, I would argue the combine function is not a good way to address the use-case I'm thinking about here, though: If you want composable drop-in objects that store their complete own state, a functional-only interface is not a good choice.

But am I right in I'm guessing the idea is that the combine function is meant to give a one-stop function, just wrapping the object-oriented interface ? That certainly makes sense, but I think it's better matched with a class heirarchy like I'm describing here, rather than the sort of partial-stateful approach the current classes implement.

eteq avatar Oct 24 '17 18:10 eteq