HEP-FCC / k4RecCalorimeter

Key4hep Framework Components for the Calorimeter Reconstruction
2 stars 18 forks source link

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

Closed dasphy closed 3 months ago

dasphy commented 4 months ago

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 commented 4 months ago

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

dasphy commented 4 months ago

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

giovannimarchiori commented 4 months ago

@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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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

kjvbrt commented 3 months ago

Can you do the same change for all the cases?

dasphy commented 3 months ago

Sounds good to me, let me modify the code accordingly

kjvbrt commented 3 months ago

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

dasphy commented 3 months ago

Thank you ! @kjvbrt