Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Software event filtering for Run 3 pp#5967

Merged
jgrosseo merged 18 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
mpuccio:trigger-devmpuccio/AliceO2:trigger-devCopy head branch name to clipboard
May 19, 2021
Merged

Software event filtering for Run 3 pp#5967
jgrosseo merged 18 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
mpuccio:trigger-devmpuccio/AliceO2:trigger-devCopy head branch name to clipboard

Conversation

@strogolo
Copy link
Contributor

This is the first version of the core event filtering framework we developed with @mpuccio.
@iarsene @jgrosseo have a look and comment!

cc: @pbuehler @pchristi @fgrosa

@strogolo strogolo closed this Apr 20, 2021
@strogolo strogolo reopened this Apr 20, 2021
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need a separate mechanism than the Configurables?

Copy link
Contributor

Choose a reason for hiding this comment

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

If Configurables support nested maps we can use them

Analysis/EventFiltering/centralEventFilterProcessor.cxx Outdated Show resolved Hide resolved
iarsene
iarsene previously approved these changes Apr 22, 2021
Analysis/EventFiltering/centralEventFilterProcessor.cxx Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

protect for 0 entries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the case of "mScalers->GetBinContent(1)" with 0 entries?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Analysis/EventFiltering/filterTables.h Outdated Show resolved Hide resolved
Analysis/EventFiltering/nucleiFilter.cxx Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

{false} should auto-expand into a n element array if I am not mistaken

Copy link
Contributor

@mpuccio mpuccio May 4, 2021

Choose a reason for hiding this comment

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

Indeed, funny thing (that burned me once) it is not true if you do it with {true}... we will fix this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jgrosseo
Copy link
Collaborator

jgrosseo commented May 4, 2021

I posted a few, mostly minor comments

@mpuccio
Copy link
Contributor

mpuccio commented May 17, 2021

Ciao @jgrosseo, I realised that we implemented you comments but we did not ping you back! We are ready for the second round of review

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a longer (more meaningful) name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done!

@jgrosseo
Copy link
Collaborator

All good for me, except the name of the executable. Could you quickly fix this, and then we start the CI?

@mpuccio
Copy link
Contributor

mpuccio commented May 18, 2021

Fixed, let's see what the CI says

jgrosseo
jgrosseo previously approved these changes May 18, 2021
@mpuccio
Copy link
Contributor

mpuccio commented May 18, 2021

Sorry, I had to update here since the PID enums were renamed while this was open and this dismissed your review @jgrosseo

@mpuccio
Copy link
Contributor

mpuccio commented May 19, 2021

CI is happy :)

@jgrosseo jgrosseo merged commit d01d21a into AliceO2Group:dev May 19, 2021
cortesep pushed a commit to cortesep/AliceO2 that referenced this pull request Jun 11, 2021
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Nov 22, 2021
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.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.