Skip to content

Conversation

@f3sch
Copy link
Collaborator

@f3sch f3sch commented Jun 7, 2024

Credit to @ddobrigk for finding it.

aod-thinner is not agnostic to the data model and when the data model for ITS clusters was changed in #12044 ITSClusterMap was effectively changed to ITSClusterSizes. Changes were not propagated to the thinner since probably at the time it was planned to only do this on 2022.

Shows depleted ITS-TPC tracks when iterating on 2023 data.
plotThin_my.pdf

Shows depletion is recovered after fix.
plotThin.pdf

Nota bene, maybe effort should be put in to make it agnostic in the future.

Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@f3sch f3sch requested a review from a team as a code owner June 7, 2024 14:21
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2024

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2022-pp-apass6-2023-PbPb-apass2
async-2022-pp-apass4
async-2022-pp-apass4-accepted
async-2022-pp-apass6-2023-PbPb-apass2-accepted
async-2023-pbpb-apass3-accepted
async-2023-pbpb-apass4-accepted
async-2023-pp-apass4
async-2023-pp-apass4-accepted
async-2024-pp-apass1
async-2024-pp-apass1-accepted
async-2022-pp-apass7
async-2022-pp-apass7-accepted
async-2024-pp-cpass0
async-2024-pp-cpass0-accepted

@f3sch
Copy link
Collaborator Author

f3sch commented Jun 7, 2024

Pinging @jgrosseo, @fgrosa, @mfaggin.

@ddobrigk
Copy link
Contributor

ddobrigk commented Jun 7, 2024

Super thanks @f3sch ! Maybe a suggestion: I think we should fatal in case bTPClsFindable==false || (bITSClusterMap==false && bITSClusterSizes==false) || bTRDPattern==false || bTOFChi2==false - because if any of these are false, we will be filtering with the default values of those parameters. It's not as smart as being data-model-agnostic but it's always better to fatal instead of silently doing something deadly, which was the case here ...

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jun 7, 2024

Well done !!

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jun 7, 2024

Super thanks @f3sch ! Maybe a suggestion: I think we should fatal in case bTPClsFindable==false || (bITSClusterMap==false && bITSClusterSizes==false) || bTRDPattern==false || bTOFChi2==false - because if any of these are false, we will be filtering with the default values of those parameters. It's not as smart as being data-model-agnostic but it's always better to fatal instead of silently doing something deadly, which was the case here ...

I agree we should do a fatal error if the needed information is not found

Fatal if necessary branches are not detected.

Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@f3sch
Copy link
Collaborator Author

f3sch commented Jun 7, 2024

Now we are fataling in case we are not finding the needed branches. Cheers, nice weekend.

@alibuild
Copy link
Collaborator

alibuild commented Jun 7, 2024

Error while checking build/O2/fullCI for 78d0606 at 2024-06-07 20:02:

No log files found

Full log here.

@ddobrigk
Copy link
Contributor

ddobrigk commented Jun 7, 2024

Awesome, thank you very much @f3sch ! And have a nice weekend!

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jun 8, 2024

@f3sch Thanks for adjusting. We mis-lead you a bit with the "fatal" word. I have adjusted to the way it is done elsewhere in that code which should return a usable error code. I neither compiled nor ran this for now. Let me know if you agree with the changes and they work for you.

@alibuild
Copy link
Collaborator

alibuild commented Jun 8, 2024

Error while checking build/O2/fullCI for f52b0a4 at 2024-06-08 11:25:

No log files found

Full log here.

jgrosseo and others added 2 commits June 8, 2024 17:07
Adjust to use exitCode
Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@f3sch f3sch force-pushed the aod/fix_thinner branch from f52b0a4 to b456154 Compare June 8, 2024 15:20
@f3sch
Copy link
Collaborator Author

f3sch commented Jun 8, 2024

Thanks @jgrosseo works as expected; I only added a newline and skipped the stat print at the end.

@jgrosseo jgrosseo enabled auto-merge (squash) June 8, 2024 19:46
@jgrosseo jgrosseo disabled auto-merge June 9, 2024 08:32
@jgrosseo jgrosseo merged commit fc75fcc into AliceO2Group:dev Jun 9, 2024
@jgrosseo
Copy link
Collaborator

jgrosseo commented Jun 9, 2024

fullCI not operational. Code currently broken. Merging.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jun 9, 2024

@chiarazampolli You can re-thin 2023 with tomorrow's tag.

@ddobrigk
Copy link
Contributor

ddobrigk commented Jun 9, 2024

Thanks! Let's still make 100% sure the 2022 was a false alarm just in case - which we can hopefully know soon, maybe already by tomorrow too.

@chiarazampolli
Copy link
Collaborator

Ok, we'll wait till tomorrow. Can we delete the 2023 thinned already?
Pinging @catalinristea, for his knowledge.

@f3sch f3sch deleted the aod/fix_thinner branch November 25, 2024 10:17
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