Skip to content

Allow OB ITSonly#12420

Merged
shahor02 merged 6 commits intoAliceO2Group:devfrom
ddobrigk:obtracksnew
Dec 11, 2023
Merged

Allow OB ITSonly#12420
shahor02 merged 6 commits intoAliceO2Group:devfrom
ddobrigk:obtracksnew

Conversation

@ddobrigk
Copy link
Copy Markdown
Contributor

@ddobrigk ddobrigk commented Dec 8, 2023

No description provided.

@ddobrigk ddobrigk requested a review from a team as a code owner December 8, 2023 13:09
Copy link
Copy Markdown
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

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

@ddobrigk note that there is already #12417 from Felix, which, contrary to your one, allows short OB tracks for photons only.
Perhaps you should coordinate with @f3sch which one to take?

}
}
} else if (itsGID.getSource() == GIndex::ITSAB) {
if (isITSTPCloaded) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ITSAB also can provide 4-point tracks (and is always OB).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes but then it is already not an itsonly track ;-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following this, I also renamed the bool into shortOBITSOnlyTrack to avoid that someone misunderstands the logic of the check that is actually done (which indeed is a bit hidden since ITSTPC tracks with 4 hits are always allowed no matter what)

@ddobrigk
Copy link
Copy Markdown
Contributor Author

ddobrigk commented Dec 8, 2023

@ddobrigk note that there is already #12417 from Felix, which, contrary to your one, allows short OB tracks for photons only. Perhaps you should coordinate with @f3sch which one to take?

Sorry, I had assumed - since I wrote a mail explicitly saying I would look at this - that I should have looked at it. Shall I close this one?

As for keeping OBs for V0s, well, I do not see any major drawback and personally I do not like a very long list of exceptions solely for photons as this kind of sounds like making the phase space more complicated for regular V0 users...

@ddobrigk ddobrigk closed this Dec 8, 2023
@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 8, 2023

Please @ddobrigk if you have time, we can use your pr. I agree that the list for photons is getting long, but if we for its ob tracks we also just have to check the photon mass. Let’s coordinate together, sorry for the confusion, somehow I did not see your mail…

@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 8, 2023

My pr is also not quiet right yet.

@ddobrigk
Copy link
Copy Markdown
Contributor Author

ddobrigk commented Dec 8, 2023

Ok, no worries at all - I also did not spot your PR, sorry for that! Anyway actually my main concern is that we add too many exceptional "allowances" to stuff that is compatible with the photon hypothesis and then analyses that do not check the photon hypothesis later on might get a mixture of populations in which these conditionals were only sometimes true. I wrote a separate mail about that concern... Curious to see what you guys think :-)

@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 8, 2023

I will close my pr and we will go further with David, sorry for the confusion.

if (isITSloaded) {
auto& itsTrack = recoData.getITSTrack(itsGID);
nITSclu = itsTrack.getNumberOfClusters();
if( itsTrack.hasHitOnLayer(6) && itsTrack.hasHitOnLayer(5) && itsTrack.hasHitOnLayer(4) && itsTrack.hasHitOnLayer(3)) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shahor02 so Felix used the snippet itsABTracklet.getFirstEntry() > 3; for this, which is admittedly much better than my ugly check, but should it not actually have been itsABTracklet.getFirstEntry() >= 3; if the first layer is layer zero?...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ddobrigk , as I wrote in the comment to the previous PR, itsABTracklet.getFirstEntry() is wrong since has nothing to do with the ITS layers: this is the entry of the tracklet's 1st cluster index in the container of indices.
In opposite, itsTrack.getFirstClusterLayer() does provide the 1st contributing layer, and for the OB tracks one should indeed use itsTrack.getFirstClusterLayer() >= 3.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah - sorry, I missed that comment. So better indeed check with getFirstClusterLayer: I've changed that! Thanks!

Please consider the following formatting changes to AliceO2Group#12420
@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 8, 2023

@shahor02 will we have to update the svparams in the ccdb with this or does the system pick up the default if it is not in the object?

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Dec 8, 2023

@f3sch new data members will be initialized according to the defaults (constructor) but since @ddobrigk has modified also the mass validation parameters, the object needs to be regenerated and uploaded again.

@ddobrigk
Copy link
Copy Markdown
Contributor Author

ddobrigk commented Dec 8, 2023

@shahor02 Ah so you mean to say that the changes to the mass I could have done via your CCDB object? That's very neat, sorry, I was not aware of that. Does this also mean that I will not necessarily have to touch anything in the O2DPG scripts? (that would be cool :-D )

@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 8, 2023

We moved the svparams to the ccdb. So everything in there we can change by changing the ccdb object, no touching anything else.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Dec 8, 2023

@ddobrigk yes, the parameters are loaded from the CCDB but can be overridden by --configKeyValues ... option from the command line. Still, it is good to have reasonable defaults set in the constructor.

@f3sch
Copy link
Copy Markdown
Collaborator

f3sch commented Dec 11, 2023

@shahor02 can we merge this? I created a jira ticket to regenerate the ccdb object https://alice.its.cern.ch/jira/browse/O2-4490.

@shahor02 shahor02 merged commit 23e9319 into AliceO2Group:dev Dec 11, 2023
chiarazampolli pushed a commit that referenced this pull request Dec 13, 2023
* Allow OB ITSonly

* Please consider the following formatting changes

* Rename to avoid confusion

* Please consider the following formatting changes

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* Allow OB ITSonly

* Please consider the following formatting changes

* Rename to avoid confusion

* Please consider the following formatting changes

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* Allow OB ITSonly

* Please consider the following formatting changes

* Rename to avoid confusion

* Please consider the following formatting changes

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
* Allow OB ITSonly

* Please consider the following formatting changes

* Rename to avoid confusion

* Please consider the following formatting changes

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
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.

4 participants