ccdproc
ccdproc copied to clipboard
Consider revising the Combiner machinery into a class heirarchy
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 acombine_images
abstract method. - Have separate
MedianCombiner
,SumCombiner
, andAverageCombiner
classes that basically just implementcombine_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 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.
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.