Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider revising the Combiner machinery into a class heirarchy #578

Open
eteq opened this issue Oct 23, 2017 · 2 comments
Open

Consider revising the Combiner machinery into a class heirarchy #578

eteq opened this issue Oct 23, 2017 · 2 comments

Comments

@eteq
Copy link
Member

eteq commented Oct 23, 2017

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

@crawfordsm
Copy link
Member

@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.

@eteq
Copy link
Member Author

eteq commented Oct 24, 2017

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.

@crawfordsm crawfordsm added this to the 2.0 milestone Oct 19, 2018
SaOgaz added a commit to SaOgaz/ccdproc that referenced this issue Jan 18, 2019
@SaOgaz SaOgaz mentioned this issue Jan 18, 2019
11 tasks
SaOgaz added a commit to SaOgaz/ccdproc that referenced this issue Jan 18, 2019
@crawfordsm crawfordsm modified the milestones: 2.0, 2.1 Jul 25, 2019
@mwcraig mwcraig modified the milestones: 2.1.0, 2.1.1 Dec 24, 2019
@mwcraig mwcraig modified the milestones: 2.1.1, 2.2.0 Mar 15, 2021
@mwcraig mwcraig modified the milestones: 2.2.0, 2.2.1 May 25, 2021
@mwcraig mwcraig modified the milestones: 2.2.1, 2.3 Nov 21, 2021
@mwcraig mwcraig modified the milestones: 2.3, 3.0 Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants