Skip to content

Conversation

@wuctlby
Copy link
Contributor

@wuctlby wuctlby commented Jul 29, 2025

Hi @zhangbiao-phy, this PR is for adding a configurable occupancy estimator in the candidate creator. Since the estimator was hard-coded as ITS from the beginning, this could cause some potential problems.

Here, I would also like to @maciacco , even though he is on vacation, in case he wants to further check something when he returns.

I already tested it locally, and everything went smoothly.

PS: I couldn't find a method to read the estimator option directly from the 'hfEvSel', so I added a new variable instead.
NOTE: If the FT0c is used, the occupancy range must also be changed accordingly.

Please let me know if you have any suggestions. Thanks a lot!

@github-actions
Copy link

github-actions bot commented Jul 29, 2025

O2 linter results: ❌ 3 errors, ⚠️ 0 warnings, 🔕 13 disabled

@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

O2 linter results: ❌ 3 errors, ⚠️ 0 warnings, 🔕 12 disabled

Regarding the errors, as I am not the developer of either code, I am not sure if it's okay that I directly disable the check for the corresponding parts.

@vkucera
Copy link
Collaborator

vkucera commented Jul 29, 2025

O2 linter results: ❌ 3 errors, ⚠️ 0 warnings, 🔕 12 disabled

Regarding the errors, as I am not the developer of either code, I am not sure if it's okay that I directly disable the check for the corresponding parts.

They are already disabled.

@vkucera vkucera marked this pull request as draft July 29, 2025 12:10
@vkucera
Copy link
Collaborator

vkucera commented Jul 29, 2025

occEstimator is already included in HfEventSelection and you literally copy-pasted its definition from there.
What's the point of this PR?

@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

occEstimator is already included in HfEventSelection and you literally copy-pasted its definition from there. What's the point of this PR?

Yes, it has in HfEventSelection. But we should also use the occEstimator, the one we want to use, in the candidate creator, instead of hard-coding it as ITS. Otherwise, we won't be able to obtain the occupancy distribution at the event level based on FT0c. In this case, if we used the FT0c in the HfEventSelection and applied the occupancy cut or a narrow occupancy range, we would only obtain the occupancy distribution at the event level based on the ITS. And here the events are those that pass the FT0c-based occupancy cut. It is not correct.

@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

occEstimator is already included in HfEventSelection and you literally copy-pasted its definition from there. What's the point of this PR?

Or, could you kindly tell me how I can read the occEstimator from the HfEventSelection instead of adding a new one in creator? I think it's better, in case someone changed the occupancy estimator in HfEventSelection but did not change it in creator. But I didn't find that there is this kind of usage.

@vkucera
Copy link
Collaborator

vkucera commented Jul 29, 2025

I see your point now.
Please get the estimator using hfEvSel.occEstimator.

@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

I see your point now. Please get the estimator using hfEvSel.occEstimator.

Yes, I also tried it. You mean something like occEstimator = hfEvSel.occEstimator?

@wuctlby wuctlby closed this Jul 29, 2025
@wuctlby wuctlby reopened this Jul 29, 2025
@vkucera
Copy link
Collaborator

vkucera commented Jul 29, 2025

I see your point now. Please get the estimator using hfEvSel.occEstimator.

Yes, I also tried it. You mean something like occEstimator = hfEvSel.occEstimator?

That should work too but I don't see why you need the new variable.

@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

I see your point now. Please get the estimator using hfEvSel.occEstimator.

Yes, I also tried it. You mean something like occEstimator = hfEvSel.occEstimator?

That should work too but I don't see why you need the new variable.

It's due to that I tried the occEstimator = hfEvSel.occEstimator, but when compiling it, it fails. So ..., I will add the variable type and try it again in the docker of ubuntu 22.04

@vkucera
Copy link
Collaborator

vkucera commented Jul 29, 2025

occEstimator

I cannot know what type your variable occEstimator is, but you can try hfEvSel.occEstimator.value which should return int.

@wuctlby wuctlby marked this pull request as ready for review July 29, 2025 15:20
@wuctlby
Copy link
Contributor Author

wuctlby commented Jul 29, 2025

Hi @vkucera , the new commit is about directly using the hfEvSel.occEstimator as the occupancy estimator in the candidate creator. Sorry, I misunderstood what you meant. I thought you were still thinking why we need to specify the estimator. After I confirmed that it can be directly used in the `getOccupancyColl, then I just got why you just asked why I need a new variable.
Please let me know if there are still any problems.

@wuctlby wuctlby changed the title [PWGHF] Add the configurable occupancy estimator in candidate creator [PWGHF] Use the option of occupancy estimator from 'HfEventSelection' in candidate creators Jul 29, 2025
@wuctlby wuctlby changed the title [PWGHF] Use the option of occupancy estimator from 'HfEventSelection' in candidate creators [PWGHF] Use the option of occupancy estimator from HfEventSelection in candidate creators Jul 29, 2025
@zhangbiao-phy
Copy link
Collaborator

zhangbiao-phy commented Jul 29, 2025

hi @wuctlby, many thanks for your commits! Maybe also modified candidateCreatorXicToXiPiPi.cxx accordingly Hi @vkucera, if you don't have further comments, I would like to approve it now. we are doing the study of D0 production vs occupancy.

@zhangbiao-phy zhangbiao-phy enabled auto-merge (squash) July 29, 2025 19:44
@zhangbiao-phy zhangbiao-phy merged commit d13f06f into AliceO2Group:master Jul 30, 2025
13 of 15 checks passed
jpxrk pushed a commit to jpxrk/O2Physics that referenced this pull request Aug 12, 2025
jloemker pushed a commit to jloemker/O2Physics that referenced this pull request Nov 11, 2025
alibuild pushed a commit to alibuild/O2Physics that referenced this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pwghf PWG-HF

Development

Successfully merging this pull request may close these issues.

3 participants