Skip to content

Conversation

@njacazio
Copy link
Collaborator

@njacazio njacazio commented Jul 2, 2021

No description provided.

@njacazio njacazio marked this pull request as ready for review July 2, 2021 10:02
@njacazio njacazio requested a review from a team as a code owner July 2, 2021 10:02
ktf
ktf previously approved these changes Jul 2, 2021
@sawenzel
Copy link
Collaborator

sawenzel commented Jul 2, 2021

@njacazio : Just for my understanding. Is this changing the data model or just the way we compute dynamic properties? (So it will not need changes in AOD conversion ?)

@njacazio
Copy link
Collaborator Author

njacazio commented Jul 2, 2021

Ciao @sawenzel good call, the AOD conversion should stay unchanged as this affects only dynamic (i.e. derived) columns

@aalkin
Copy link
Member

aalkin commented Jul 4, 2021

@njacazio changing eta to an expression column would lead to division by zero for some of the particles, I've tried it :)

@njacazio
Copy link
Collaborator Author

njacazio commented Jul 5, 2021

@aalkin is there a protection that we can add to avoid that? Is the division avoided with dynamic columns?

@aalkin
Copy link
Member

aalkin commented Jul 5, 2021

@aalkin is there a protection that we can add to avoid that? Is the division avoided with dynamic columns?

With dynamic columns this is avoided simply because those are called only for some particles useful in analysis - with a reasonable values of pseudorapidity. Expression would run for every particles, including those very forward ones. Before we have conditional nodes in expressions or an ability to use arrow's null rows I'm afraid there is no workaround.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jul 5, 2021

We do not trap FPE in O2 to my understanding. What is the problem here?

@aalkin
Copy link
Member

aalkin commented Jul 5, 2021

We do not trap FPE in O2 to my understanding. What is the problem here?

This is outside O2, this is done by Gandiva library and it stops on FPE.

@njacazio
Copy link
Collaborator Author

njacazio commented Jul 5, 2021

@aalkin I see. So, for now, we should leave Eta as a dynamic column right?

@aalkin
Copy link
Member

aalkin commented Jul 6, 2021

@aalkin I see. So, for now, we should leave Eta as a dynamic column right?

Yes, unfortunately that is correct.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jul 6, 2021

This is a severe limitation. When could we have conditional nodes in gandiva?

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jul 6, 2021

In the meanwhile we may think about adjusting slightly the momenta of the particles where we get fpe. These will be outside the physical region anyway, I suspect...

@njacazio
Copy link
Collaborator Author

njacazio commented Jul 6, 2021

@aalkin I see. So, for now, we should leave Eta as a dynamic column right?

Yes, unfortunately that is correct.

And we cannot pre-cook variables in dynamic columns and then feed them to expression columns for the heavy lifting right?

@aalkin
Copy link
Member

aalkin commented Jul 6, 2021

And we cannot pre-cook variables in dynamic columns and then feed them to expression columns for the heavy lifting right?

One could indeed create a non-dynamic column, fill it values using a normal task with safety checks and then append expression column that uses these new numbers.

@njacazio
Copy link
Collaborator Author

With these two commits alisw/AliPhysics#18164 and #6691 we should be able to avoid the FPE until we have the extension in the features

@ktf
Copy link
Member

ktf commented Jul 21, 2021

Do we have any reason for not merging this? Yes, the PRs in AliPhysics are not yet merged.

@njacazio
Copy link
Collaborator Author

Ciao @ktf I don't know, but to be working with AO2Ds we first need the fixes in AliPhysics and O2 workflows

Copy link
Collaborator

@jgrosseo jgrosseo left a comment

Choose a reason for hiding this comment

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

We have to reproduce the MCs first. Do not merge, yet.

@njacazio
Copy link
Collaborator Author

Both PRs pending are now merged, we wait for the test on converted data and then we go on!

@aalkin
Copy link
Member

aalkin commented Jul 28, 2021

@njacazio #6753 just got merged, you can now try making expressions with ifnode(condition_exp, then_exp, else_exp)

@njacazio
Copy link
Collaborator Author

@aalkin brilliant! I'm on it!! Thanks a lot

@njacazio njacazio force-pushed the nj-expr-col branch 3 times, most recently from 86537e7 to f99a824 Compare July 28, 2021 14:04
@njacazio
Copy link
Collaborator Author

I tested that this works on LHC18g4 285011 181_20210626-1348 ready for review!

Copy link
Collaborator

@jgrosseo jgrosseo left a comment

Choose a reason for hiding this comment

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

As discussed, let's have hard coded values when it would trigger an FPE.

- Add ifnode for y and eta to avoid FPEs
@jgrosseo jgrosseo merged commit aacf2cb into AliceO2Group:dev Aug 2, 2021
skundu692 pushed a commit to skundu692/AliceO2 that referenced this pull request Aug 10, 2021
- Add ifnode for y and eta to avoid FPEs
@njacazio njacazio deleted the nj-expr-col branch March 16, 2022 12:53
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