Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Photon ID shape parameter: correct very small negative values caused by computational precision in width of theta/module #90

Merged
merged 38 commits into from
Jul 24, 2024

Conversation

dasphy
Copy link
Contributor

@dasphy dasphy commented Jul 4, 2024

Follow-up of #79 . In the width calculation:
double _w_theta_3Bin = sqrt(theta2_E_3Bin / sum_E_3Bin - std::pow(theta_E_3Bin / sum_E_3Bin, 2));
Mathematically the value inside sqrt() should never be negative, but a very small negative value might be produced, due to the precision of calculation in code.

A threshold -1e-9 is set to correct this:
if (_w_theta_3Bin2 < 0. && _w_theta_3Bin2 > -1e-9) _w_theta_3Bin2 = 0. ;

Tagging @giovannimarchiori , @kjvbrt

Cheers, Tong

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 5, 2024

Hi @dasphy,
can you instead of if (_w_theta_3Bin2 < 0. && _w_theta_3Bin2 > -1e-9) use just std::fabs()?

@dasphy
Copy link
Contributor Author

dasphy commented Jul 7, 2024

Yes I think fabs() is better. I just pushed changes.

@giovannimarchiori
Copy link
Contributor

@kjvbrt I don't like too much this solution, without even adding a comment to the code to explain the fabs, nor a warning in the output. I would have preferred the option that if the argument of sqrt is slightly negative the width is set to zero and a warning message is print to log, giving also the value of wtheta2.
With fabs we're trusting the fact that wtheta2 is negative only because of small rounding issues and not because of some buggy calculation upstream.

@dasphy
Copy link
Contributor Author

dasphy commented Jul 11, 2024

I added warnings in the log and set the value to 0 if w_theta2 is slightly negative. please have a look at the changes.
After testing 100,000 events, w_theta2 has never been a negative value less than -1e-9. The slightly negative values were caused by calculation precision in the code.
Cheers, Tong

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 24, 2024

Hi, what about having a hard crash or warning if the variable is negative more that -1e-9 and otherwise use std::fabs?

The test now notifies only when the negative value is very small.

@giovannimarchiori
Copy link
Contributor

Hi, what about having a hard crash or warning if the variable is negative more that -1e-9 and otherwise use std::fabs?

The test now notifies only when the negative value is very small.

Seems a good idea to me

Co-authored-by: Juraj Smiesko <[email protected]>
@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 24, 2024

Can you do the same change for all the cases?

@dasphy
Copy link
Contributor Author

dasphy commented Jul 24, 2024

Sounds good to me, let me modify the code accordingly

@kjvbrt
Copy link
Contributor

kjvbrt commented Jul 24, 2024

Nice, looks good to me. Will merge shortly...

@dasphy
Copy link
Contributor Author

dasphy commented Jul 24, 2024

Thank you ! @kjvbrt

@kjvbrt kjvbrt merged commit 1ae30cf into HEP-FCC:main Jul 24, 2024
0 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants