mezuro / kalibro_processor

Reimplementation of Kalibro processing
GNU Affero General Public License v3.0
2 stars 10 forks source link

Refactor ModuleResult#pre_order #222

Open rafamanzo opened 8 years ago

rafamanzo commented 8 years ago

This is an optimization made by @danielkza at 805c7c139b92afa3c3dc056ee84fe35473e55500 which reduces the complexity of finding the root finding it through the Processing.

I'm not sure the memozation through the attribute @pre_order worked at and if we should keep it.

@danielkza have a look if you agree with this extraction and @mezuro/core we need a third pair of eyes here as this is a derivative I've made from @danielkza's work.

It is is a slight improvement for https://github.com/mezuro/kalibro_processor/issues/207 that we've to check the impact.

danielkza commented 8 years ago

:+1: for me

rafamanzo commented 8 years ago

After https://github.com/mezuro/kalibro_processor/pull/225 where will pre_order still be used? Maybe is a better investment to refactor what still uses this method in terms the level walking methods introduced there and completely remove this method. What do you think?

danielkza commented 8 years ago

That is my plan. As far as I can determine, the only processing step that really cares about navigating the tree from children to parents is the tree builder, but it obviously can't use pre-order as it has to create the tree.

The other steps either don't care about order at all (compound calculator, interpreter, checker), or can work with level-order (aggregator).

I was also thinking: if we were willing to refactor all the steps, we could get away with only fetching results from the database once, after the collecting step. Since only the corresponding job should modify results of a processing before it is done, we can be sure our in-memory representation of the tree is accurate, as long as we create all the new records using build_ methods.

diegoamc commented 8 years ago

Needs rebasing.

rafamanzo commented 8 years ago

Not sure if we want to merge this or just get rid of pre_order. I'm inclined to the later. What do you think @diegoamc ?

The other uses of this method are Compound calculator and Interpreter.