Skip to content

[QC-326] Aggregators#488

Merged
Barthelemy merged 41 commits into
AliceO2Group:masterfrom
Barthelemy:aggregators
Nov 23, 2020
Merged

[QC-326] Aggregators#488
Barthelemy merged 41 commits into
AliceO2Group:masterfrom
Barthelemy:aggregators

Conversation

@Barthelemy
Copy link
Copy Markdown
Collaborator

No description provided.

@Barthelemy Barthelemy marked this pull request as draft September 22, 2020 10:15
@Barthelemy Barthelemy force-pushed the aggregators branch 2 times, most recently from c488567 to 57fdda3 Compare November 12, 2020 14:59
@Barthelemy Barthelemy changed the title Aggregators [WIP] Aggregators Nov 17, 2020
@Barthelemy
Copy link
Copy Markdown
Collaborator Author

Still a bit of work to be done on the doxygen.

@Barthelemy Barthelemy changed the title [WIP] Aggregators Aggregators Nov 17, 2020
@Barthelemy Barthelemy requested a review from knopers8 November 17, 2020 15:39
@Barthelemy Barthelemy changed the title Aggregators [QC-326] Aggregators Nov 18, 2020
Copy link
Copy Markdown
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have a lot of small suggestions and questions, but in total, I think it is very good.
I guess that the next step would be to make some common aggregator like WorstFromAll.

I am still a bit puzzled what should happen an aggregator depends on another aggregator. Is it guaranteed that it will be invoked in the same run()?
I am also wondering about circular dependencies among aggregators - this might be a rare case, but perhaps we should check against that to warn a user. Maybe not in this iteration, but later.

Comment thread doc/ModulesDevelopment.md Outdated
Comment thread doc/ModulesDevelopment.md Outdated
Comment thread doc/ModulesDevelopment.md Outdated
Comment thread doc/ModulesDevelopment.md Outdated
Comment thread doc/ModulesDevelopment.md Outdated
Comment thread Framework/src/Aggregator.cxx Outdated
Comment thread Framework/src/AggregatorRunner.cxx Outdated
Comment thread Framework/src/AggregatorRunner.cxx Outdated
Comment thread Framework/src/AggregatorRunner.cxx Outdated
Comment thread Framework/src/AggregatorRunner.cxx Outdated
@Barthelemy
Copy link
Copy Markdown
Collaborator Author

Thank you. I have a lot of small suggestions and questions, but in total, I think it is very good.
I guess that the next step would be to make some common aggregator like WorstFromAll.

Great review, thank you !
I am addressing the suggestions step by step.

Yes, I have create https://alice.its.cern.ch/jira/browse/QC-488. I have assigned it to you for the time being with the idea that it might be beneficial that someone else tries out the aggregators. However, I can take it back if needed.

I am still a bit puzzled what should happen an aggregator depends on another aggregator. Is it guaranteed that it will be invoked in the same run()?

No, it is not guaranteed. If they are defined in a non optimal order, then you could have to wait for the next call to run(). It means that in this case, the aggregator will have to work with the quality of the last cycle, i.e. that it there will be a kind of delay. We could improve it by creating a tree and making them run in the correct order. I would postpone that for a future improvement but I will add a comment in the documentation. https://alice.its.cern.ch/jira/browse/QC-489

I am also wondering about circular dependencies among aggregators - this might be a rare case, but perhaps we should check against that to warn a user. Maybe not in this iteration, but later.

I considered circular dependencies and it should not be a disaster for the policy OnAny because the aggregators have access to the quality of the previous cycle in the worst case (we maintain the container). For "OnAll" it could be a problem because it will never trigger. I propose to treat it in QC-489 as well.

@knopers8
Copy link
Copy Markdown
Collaborator

Yes, it would be good if I played with it as well, the ticket can stay with me.
Agreed for the other issues as well.

Comment thread doc/ModulesDevelopment.md Outdated
Comment thread Framework/src/InfrastructureGenerator.cxx
@Barthelemy
Copy link
Copy Markdown
Collaborator Author

I think that I fixed most of the things. There are 3 open items that I let you have a look at on Monday.

@Barthelemy Barthelemy merged commit 903d1c8 into AliceO2Group:master Nov 23, 2020
@Barthelemy Barthelemy deleted the aggregators branch November 23, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants